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.

Sign up for GitHub

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

Define useful celery beat task for development #3762

Merged
merged 5 commits into from
Mar 23, 2018
Merged

Define useful celery beat task for development #3762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cb4ef6
Task to remove orphan symlinks
humitos Jan 23, 2018
e035eac
Use a better db query to accomplish this task
humitos Mar 9, 2018
2555fe0
Setup task to be ran by celery beat
humitos Mar 10, 2018
b68ea63
Register celery beat task useful for development
humitos Mar 10, 2018
7f062f9
Merge branch 'master' into humitos/celerybeat/definetask
agjohnson Mar 23, 2018
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
18 changes: 18 additions & 0 deletions readthedocs/projects/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,24 @@ def symlink_domain(project_pk, domain_pk, delete=False):
sym.symlink_cnames(domain)


@app.task(queue='web')
def remove_orphan_symlinks():
"""
Remove orphan symlinks.

List CNAME_ROOT for Public and Private symlinks, check that all the listed
cname exist in the database and if doesn't exist, they are un-linked.
"""
for symlink in [PublicSymlink, PrivateSymlink]:
for domain_path in [symlink.PROJECT_CNAME_ROOT, symlink.CNAME_ROOT]:
valid_cnames = set(Domain.objects.all().values_list('domain', flat=True))
orphan_cnames = set(os.listdir(domain_path)) - valid_cnames
for cname in orphan_cnames:
orphan_domain_path = os.path.join(domain_path, cname)
log.info('Unlinking orphan CNAME: %s', orphan_domain_path)
os.unlink(orphan_domain_path)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these should go in another file. projects/tasks is already huge, so it might make sense to move this into core/tasks.py or somewhere else specifically for scheduled tasks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I agree with this.

I created this issue (#3775) to refactor this and include some other tasks also --we can discuss in the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually dislike putting things in core. Why not split up into:

  • readthedocs.projects.tasks.build
  • readthedocs.projects.tasks.sync
  • etc

Some of the tasks might make more sense in builds/ as well. I find core/ is not a helpful concept, save for only the most generalized of code.


@app.task(queue='web')
def symlink_subproject(project_pk):
project = Project.objects.get(pk=project_pk)
Expand Down
75 changes: 74 additions & 1 deletion readthedocs/rtd_tests/tests/test_project_symlinks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from readthedocs.builds.models import Version
from readthedocs.projects.models import Project, Domain
from readthedocs.projects.tasks import symlink_project
from readthedocs.projects.tasks import symlink_project, remove_orphan_symlinks
from readthedocs.core.symlink import PublicSymlink, PrivateSymlink


Expand Down Expand Up @@ -166,6 +166,79 @@ def test_symlink_cname(self):
filesystem['private_web_root'] = public_root
self.assertFilesystem(filesystem)

def test_symlink_remove_orphan_symlinks(self):
self.domain = get(Domain, project=self.project, domain='woot.com',
url='http://woot.com', cname=True)
self.symlink.symlink_cnames()

# Editing the Domain and calling save will symlink the new domain and
# leave the old one as orphan.
self.domain.domain = 'foobar.com'
self.domain.save()

filesystem = {
'private_cname_project': {
'foobar.com': {'type': 'link', 'target': 'user_builds/kong'},
'woot.com': {'type': 'link', 'target': 'user_builds/kong'},
},
'private_cname_root': {
'foobar.com': {'type': 'link', 'target': 'private_web_root/kong'},
'woot.com': {'type': 'link', 'target': 'private_web_root/kong'},
},
'private_web_root': {'kong': {'en': {}}},
'public_cname_project': {
'foobar.com': {'type': 'link', 'target': 'user_builds/kong'},
'woot.com': {'type': 'link', 'target': 'user_builds/kong'},
},
'public_cname_root': {
'foobar.com': {'type': 'link', 'target': 'public_web_root/kong'},
'woot.com': {'type': 'link', 'target': 'public_web_root/kong'},
},
'public_web_root': {
'kong': {'en': {'latest': {
'type': 'link',
'target': 'user_builds/kong/rtd-builds/latest',
}}}
}
}
if self.privacy == 'private':
public_root = filesystem['public_web_root'].copy()
private_root = filesystem['private_web_root'].copy()
filesystem['public_web_root'] = private_root
filesystem['private_web_root'] = public_root
self.assertFilesystem(filesystem)

remove_orphan_symlinks()
filesystem = {
'private_cname_project': {
'foobar.com': {'type': 'link', 'target': 'user_builds/kong'},
},
'private_cname_root': {
'foobar.com': {'type': 'link', 'target': 'private_web_root/kong'},
},
'private_web_root': {'kong': {'en': {}}},
'public_cname_project': {
'foobar.com': {'type': 'link', 'target': 'user_builds/kong'},
},
'public_cname_root': {
'foobar.com': {'type': 'link', 'target': 'public_web_root/kong'},
},
'public_web_root': {
'kong': {'en': {'latest': {
'type': 'link',
'target': 'user_builds/kong/rtd-builds/latest',
}}},
},
}
if self.privacy == 'private':
public_root = filesystem['public_web_root'].copy()
private_root = filesystem['private_web_root'].copy()
filesystem['public_web_root'] = private_root
filesystem['private_web_root'] = public_root

self.assertFilesystem(filesystem)


def test_symlink_cname_dont_link_missing_domains(self):
"""Domains should be relinked after deletion"""
self.domain = get(Domain, project=self.project, domain='woot.com',
Expand Down
16 changes: 16 additions & 0 deletions readthedocs/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

from readthedocs.core.settings import Settings

from celery.schedules import crontab


try:
import readthedocsext # noqa
ext = True
Expand Down Expand Up @@ -241,6 +244,19 @@ def USE_PROMOS(self): # noqa
CELERY_CREATE_MISSING_QUEUES = True

CELERY_DEFAULT_QUEUE = 'celery'
CELERYBEAT_SCHEDULE = {
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure we aren't overridding this elsewhere. We need a good pattern here, but I'm guessing we aren't extending this everywhere else we use it, so this might get killed by a downstream config file overwriting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found we use this setting in 3 places:

@ericholscher please confirm this, and I will open a PR for corporate ops to modify this file https://github.com/readthedocs/readthedocs-corporate-ops/blob/master/salt/corporate/readthedocsinc/settings/base.py

Copy link
Contributor

Choose a reason for hiding this comment

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

You need a property override there as well, correct.

# Ran every hour on minute 30
'hourly-remove-orphan-symlinks': {
'task': 'readthedocs.projects.tasks.remove_orphan_symlinks',
'schedule': crontab(minute=30),
'options': {'queue': 'web'},
},
'quarter-finish-inactive-builds': {
'task': 'readthedocs.projects.tasks.finish_inactive_builds',
'schedule': crontab(minute='*/15'),
'options': {'queue': 'web'},
},
}

# Docker
DOCKER_ENABLE = False
Expand Down