Skip to content

SE-3149 Fix inconsistent cached file hash issue since juniper - #24990

Merged
kdmccormick merged 1 commit into
openedx:masterfrom
open-craft:pooja/fix-theme-compilation-issue
Dec 11, 2020
Merged

SE-3149 Fix inconsistent cached file hash issue since juniper#24990
kdmccormick merged 1 commit into
openedx:masterfrom
open-craft:pooja/fix-theme-compilation-issue

Conversation

@pkulkark

@pkulkark pkulkark commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

CachedStaticFilesStorage is deprecated in Django 2.2. It's plugin model i.e. CachedFilesMixin was being used in theme asset compilation. This was leading to inconsistent theme asset compilation and would lead to failures if more than one instance is used for load-balancing. This PR replaces the deprecated CachedFilesMixin with ManifestFilesMixin. This also includes the fix for Webpack's cached files which also had similar issue where the hashes were not consistent on different machines and would change on every assets update. Accordingly, this caused 404 errors with multiple instances and load balancers, which made the Studio and LMS unusable.

Jira Tickets: OSPR-4974

Dependecies: None

Sandbox URL: TBD - sandbox is being provisioned

Merge deadline: None

Testing Instructions:

  1. Spawn a new edX juniper instance and check the css files generated in /edx/var/edxapp/staticfiles/css.
  2. Run paver update_assets command and recheck that no new css files are generated in /edx/var/edxapp/staticfiles/css.
  3. Spawn multiple instances and add to load-balancer. Verify that the assets are getting loaded correctly.
  4. Open the Developer console for the LMS and Studio and verify there are no 404 messages for commons.js.

Reviewers:

@openedx-webhooks

openedx-webhooks commented Sep 16, 2020

Copy link
Copy Markdown

Thanks for the pull request, @pkulkark! I've created OSPR-4974 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@pkulkark

Copy link
Copy Markdown
Contributor Author

jenkins run a11y

@pkulkark

Copy link
Copy Markdown
Contributor Author

@natabene This is good for edX's review.

@Kelketek

Copy link
Copy Markdown
Contributor

👍

  • I tested this: Verified that this produces deterministic asset builds across multiple machines.
  • I read through the code

@natabene

Copy link
Copy Markdown
Contributor

@pkulkark Thank you, lining this up for our review.

@pkulkark pkulkark changed the title SE-3149 Fix the theme compilation issue since juniper SE-3149 Fix inconsistent cached file hash issue since juniper Oct 9, 2020
@nizarmah

nizarmah commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

@pkulkark is there a change we can mark this as "not ready" for now? There is still a small change that needs to be done to the Jakefile in vendor_extra/tinymce/JakePackage.zip

@pkulkark

pkulkark commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

@nizarmah Since this is already line up for edX's review, I'd suggest raising a separate PR for the Jakefile fix.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@natabene Could I take this review?

@natabene

natabene commented Nov 3, 2020

Copy link
Copy Markdown
Contributor

@bradenmacdonald Sure.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@pkulkark This looks nice, thanks. Is there a sandbox I can use to test this? Or should I test on my devstack? Also, the test instructions say "juniper instance" but I assume I can use this (master) branch right?

@pkulkark

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald Yes you can use this (master) branch. I initially had raised the PR for juniper branch and forgot to change that. As for the sandbox, you can test it on https://manage.opencraft.com/instance/21210/. Sorry for not updating the description properly.

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, nice work! Sorry for the slow review on my side.

👍

  • I tested this: compiled assets on devstack and on pr24990.sandbox.opencraft.hosting sandbox, did smoke test
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

Run paver update_assets command and recheck that no new css files are generated in /edx/var/edxapp/staticfiles/css.

I tried this on pr24990.sandbox.opencraft.hosting but it gave an error about django debug toolbar being missing. I had to run paver update_assets lms --settings=production instead.

When testing this, I noticed Skipping theme asset compilation: enable theming to process themed assets in the build output. So I'm not sure that I tested the parts that involve static asset files for themes.

Could you please:

  • Rebase this on latest master, fix the tiny little nits I mentioned, and squash to a single commit with a nice commit message
  • Test that final commit on either devstack or an Ocim instance, with a theme installed, just to make sure the theme asset compilation is working correctly and there are no conflicts/bugs after being rebased with master.

And then I'll get this merged!

Comment thread openedx/core/djangoapps/theming/storage.py Outdated
Comment thread common/lib/xmodule/xmodule/static_content.py Outdated
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.
@pkulkark
pkulkark force-pushed the pooja/fix-theme-compilation-issue branch from 7bd0980 to cb5407e Compare November 25, 2020 10:30
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@kdmccormick

Copy link
Copy Markdown
Member

Thanks for the fix @pkulkark and @bradenmacdonald . I'll merge this today or tomorrow, depending on how the edxapp deployment pipeline is looking.

@kdmccormick

Copy link
Copy Markdown
Member

The deployment pipeline is unfortunately still blocked. Going to hold off on this, but I'll merge it as soon as we're shipping to prod again.

@kdmccormick

Copy link
Copy Markdown
Member

Looks like this have stabilized as of this afternoon; I'll merge this first thing tomorrow morning (ET).

@kdmccormick

kdmccormick commented Dec 11, 2020

Copy link
Copy Markdown
Member

@pkulkark @bradenmacdonald Merging this now; I'd estimate it to be on Stage by 10:15am EST and Prod by 11am EST.

@kdmccormick
kdmccormick merged commit e9cba8c into openedx:master Dec 11, 2020
@openedx-webhooks

Copy link
Copy Markdown

@pkulkark 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@pomegranited
pomegranited deleted the pooja/fix-theme-compilation-issue branch December 23, 2020 03:09
@pomegranited

Copy link
Copy Markdown
Contributor

Thank you for reviewing and merging this @bradenmacdonald and @kdmccormick !

@nedbat Can this be cherry-picked into Koa? CC @pkulkark

nizarmah pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 20, 2021
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.

(cherry picked from commit e9cba8c)
0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Feb 15, 2021
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.

(cherry picked from commit e9cba8c)
0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Apr 20, 2021
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.

(cherry picked from commit e9cba8c)
MoisesGSalas pushed a commit to eduNEXT/edx-platform that referenced this pull request Oct 27, 2021
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.

(cherry picked from commit e9cba8c)
DonatoBD pushed a commit to eduNEXT/edx-platform that referenced this pull request Jan 17, 2022
Replaces the deprecated CachedFilesMixin with ManifestFilesMixin.
Also sorts xmodules and xblocks js bundles to make theme assets
compilation hashes consistent across different machines.

(cherry picked from commit e9cba8c)
DonatoBD added a commit to eduNEXT/edx-platform that referenced this pull request Jan 17, 2022
Fix the theme compilation issue since juniper (openedx#24990)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants