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 code replaced by El Proxito and stateless servers #6535

Merged
merged 19 commits into from May 6, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 18, 2020

After working on #6326 and realizing that PR will introduce a breaking change, I started playing around by removing more code that we know is not going to be needed once El Proxito is fully rolled out. The idea is to make just one breaking change with all of these remotions together if that is possible. This PR removes these,

  • Syncers: all the files generated by the builders are copied directly from there to the Django Storage backend configured
  • Some web server tasks: there is no need to remove, sync or copy files anymore since these servers won't have any documentation or state on them
  • Symlink code: files are not served from disk anymore. They are served from storage backend via El Proxito
  • Subdomain and SingleVersion logic + urls: this is now handled by El Proxito

This PR will introduce a big breaking change and force core team and contributors to use our docker-compose based solution for their development environment.

humitos added 3 commits Jan 18, 2020
* `remove_dirs` is not needed to be executed anymore on webs because
all the artifacts are saved on django storage backend now. So, this
task is being replaced by `clean_project_resources`.

* `move_files` is not needed because we don't need to sync files
anymore to the web servers. Everything is uploaded from build servers
to the django storage backend now.

* broadcast finalization steps are replaced by only a call to
`filefy`. The rest are not required anymore.

* `update_static_metadata` won't be needed because the redirection
done by the Flask app it will be replaced by El Proxito.

* Syncers are removed completely. This copying is happening on build
servers now and pushing the files to django storage backend.
We are serving documentation from El Proxito now and these middlewares
are not required.

The ability to server at `/docs/` is removed as well.
@humitos humitos added the PR: work in progress label Jan 18, 2020
@humitos humitos force-pushed the humitos/remove-web-related-tasks branch from dc5448b to 4d54072 Compare Jan 18, 2020
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 21, 2020

Some web server tasks: there is no need to remove, sync or copy files anymore since these servers won't have any documentation or state on them

We still need to run these tasks, no? Just not on the web servers but in cloud storage?

@humitos
Copy link
Member Author

@humitos humitos commented Jan 21, 2020

We still need to run these tasks, no? Just not on the web servers but in cloud storage?

This is done inside the UpdateDocsTaskStep that runs in the builder now, at this method

def store_build_artifacts(
self,
html=False,
localmedia=False,
search=False,
pdf=False,
epub=False,
):
"""
Save build artifacts to "storage" using Django's storage API.
The storage could be local filesystem storage OR cloud blob storage
such as S3, Azure storage or Google Cloud Storage.

(there is a call to storage.sync_directory inside that method)

@stale
Copy link

@stale stale bot commented Mar 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Mar 6, 2020
@stale stale bot closed this Mar 13, 2020
@humitos
Copy link
Member Author

@humitos humitos commented Mar 13, 2020

Let's un-stale this PR for now. We are getting close to be able to remove all this code. We will be pinged back by stale bot in a month or so.

@humitos humitos reopened this Mar 13, 2020
@stale stale bot removed the Status: stale label Mar 13, 2020
@stale
Copy link

@stale stale bot commented Apr 28, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Apr 28, 2020
@stale stale bot removed the Status: stale label Apr 28, 2020
@humitos humitos removed the PR: work in progress label Apr 28, 2020
@humitos
Copy link
Member Author

@humitos humitos commented Apr 28, 2020

I updated the PR with the latest changes. We are getting closer to be able to remove all this code.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 28, 2020

Yea, I'd like to get the symlink code removed this week if possible. It should make our tests a lot less spammy.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 29, 2020

@humitos looks like CI is unhappy with this, so still some work to do.

Copy link
Member

@ericholscher ericholscher left a comment

Excited for this :)

def form_valid(self, form):
broadcast(
type='app',
task=tasks.symlink_subproject,
Copy link
Member

@ericholscher ericholscher Apr 29, 2020

Choose a reason for hiding this comment

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

I think a lot of the places we were calling symlink code we actually want to bust the cache for the project as well. Something to consider, though maybe we want to add that logic in a new PR after this.

Copy link
Member Author

@humitos humitos Apr 29, 2020

Choose a reason for hiding this comment

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

I agree. In this particular case it was called when de-activating a version. The following line (which I didn't add) triggers a clean_project_resources which removes the version from the storage. Seems like someone already thought about this 😄

Copy link
Member Author

@humitos humitos Apr 29, 2020

Choose a reason for hiding this comment

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

@@ -190,8 +185,6 @@ def USE_PROMOS(self): # noqa
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'dj_pagination.middleware.PaginationMiddleware',
'readthedocs.core.middleware.SubdomainMiddleware',
'readthedocs.core.middleware.SingleVersionMiddleware',
Copy link
Member

@ericholscher ericholscher Apr 29, 2020

Choose a reason for hiding this comment

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

I think we replace this in the proxito code, so probably need to update that as well.

@@ -577,7 +561,6 @@ def USE_PROMOS(self): # noqa
GRAVATAR_DEFAULT_IMAGE = 'https://assets.readthedocs.org/static/images/silhouette.png' # NOQA
OAUTH_AVATAR_USER_DEFAULT_URL = GRAVATAR_DEFAULT_IMAGE
OAUTH_AVATAR_ORG_DEFAULT_URL = GRAVATAR_DEFAULT_IMAGE
RESTRICTEDSESSIONS_AUTHED_ONLY = True
Copy link
Member

@ericholscher ericholscher Apr 29, 2020

Choose a reason for hiding this comment

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

This seems unrelated?

Copy link
Member Author

@humitos humitos Apr 29, 2020

Choose a reason for hiding this comment

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

Hrm... I'm not sure how this ended up here. I could re-add it, but we don't use it anymore, though. It's from https://github.com/mxsasha/django-restricted-sessions

'readthedocs.core.middleware.SubdomainMiddleware'
)
classes[index] = 'readthedocs.proxito.middleware.ProxitoMiddleware'
classes.append('readthedocs.proxito.middleware.ProxitoMiddleware')
Copy link
Member

@ericholscher ericholscher Apr 29, 2020

Choose a reason for hiding this comment

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

👍

humitos added 4 commits Apr 29, 2020
`ln -nsf` is not called anymore when building docs.
We do not use `/docs/` anymore. We have more similar tests under
rtd_tests/tests/test_resolver.py that are better.
Most of these files were migrated to
`readthedocs/proxito/tests/test_old_redirects.py` and they are using the new
settings and behaves according production.

We could remove this file (`readthedocs/rtd_tests/tests/test_redirects.py`)
completely probably. For now, I'm just removing the tests that are failing, tho
@humitos humitos marked this pull request as ready for review Apr 29, 2020
@humitos
Copy link
Member Author

@humitos humitos commented Apr 29, 2020

I worked on this to fix all the tests. I found and fixed these:

  • some popen count changed because we are not running ln -ns anymore
  • removed resolver tests using /docs/ prefix --they were already migrated to rtd_tests/tests/test_resolver.py already
  • removed some redirects started failing because they use old /docs/ prefix --they were already migrated to readthedocs/rtd_tests/tests/test_redirects.py already

There are only 2 tests that are failing:

  • is happening on hg pull, and I'm not sure why yet
  • private doc serving under /docs/django-kong/ --which I'm not sure if I should remove it directly

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 29, 2020

private doc serving under /docs/django-kong/ --which I'm not sure if I should remove it directly

Yea, we shouldn't support those URL's anymore, so we don't need tests for them 👍 If we can migrate the test to the subdomain URL thats good, if there already is one, delete the /docs/ one.

humitos added 3 commits Apr 30, 2020
This was only working on /docs/ paths. Now, with El Proxito we are not
respecting the privacy level in community site.
Copy link
Member

@ericholscher ericholscher left a comment

I'm 👍 on this. We should merge it soon so that it doesn't diverge much from the main codeline. @readthedocs/core, if folks could take a look today, I think it makes sense to try and get this shipped for next week.

The other option I see if to merge it right after deploy next week -- that way we have some time for it to settle in.

Copy link
Contributor

@davidfischer davidfischer left a comment

I'm in favor of merging this although I don't think I'm fully qualified to review every aspect. I'm glad to see all the syncer code go, however.

@@ -22,7 +21,6 @@

admin.autodiscover()

handler404 = server_error_404
Copy link
Contributor

@davidfischer davidfischer May 1, 2020

Choose a reason for hiding this comment

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

I think you still want this. This is to handle dashboard 404s too, I believe.

Copy link
Member Author

@humitos humitos May 4, 2020

Choose a reason for hiding this comment

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

Regarding the 404, we are using the Django default on dashboard: https://docs.djangoproject.com/en/2.2/ref/views/#django.views.defaults.page_not_found --It keeps showing our custom 404.html Maze Found.

Our custom handler is not more required because we are not doing any redirect now, just showing the Maze.

Copy link
Member Author

@humitos humitos May 4, 2020

Choose a reason for hiding this comment

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

I just checked that running the code with DEBUG=False still shows the Maze Found page on any 404 page under the dashboard.

@@ -46,8 +44,6 @@
url(r'^accounts/gold/', include('readthedocs.gold.urls')),
# For redirects
url(r'^builds/', include('readthedocs.builds.urls')),
# For testing the 404's with DEBUG on.
url(r'^404/$', handler404),
Copy link
Contributor

@davidfischer davidfischer May 1, 2020

Choose a reason for hiding this comment

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

These enable the following URLs:

These are pretty useful in debugging and maybe they should be in the debug_urls. I don't think we should remove just one of them though.

Copy link
Member Author

@humitos humitos May 4, 2020

Choose a reason for hiding this comment

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

I didn't remove 500. That is still working.

Copy link
Member Author

@humitos humitos May 4, 2020

Choose a reason for hiding this comment

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

/404/ will use the default Django handler and it should keep working normally as any other not found URL.

humitos added 2 commits May 4, 2020
Showing it when DEBUG=False fails because we are not registering the URLs on
DEBUG=False. Also, leaving just the default value does not work because it
checks for the IP to be an INTERNAL_IP, which is not the case on Docker.
@stsewd
Copy link
Member

@stsewd stsewd commented May 4, 2020

This can be removed too

def get_production_media_path(self, type_, version_slug, include_file=True):

stsewd
stsewd approved these changes May 4, 2020
Copy link
Member

@stsewd stsewd left a comment

Coverage actually decreased to 81.2% ha :(

https://codecov.io/gh/readthedocs/readthedocs.org/pull/6535/diff

@humitos
Copy link
Member Author

@humitos humitos commented May 6, 2020

I'm going to merge this so we have 1 week testing it before deploying. Yay! 🎉

@humitos humitos merged commit 238ef8b into master May 6, 2020
2 checks passed
@humitos humitos deleted the humitos/remove-web-related-tasks branch May 6, 2020
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.

None yet

4 participants