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

Issue/279 heroku background worker #292

Merged

Conversation

iturgeon
Copy link
Member

@iturgeon iturgeon commented Jun 7, 2017

The main goal of this PR is to add a background worker mode to UDOIT. This allows UDOIT to support more simultaneous users and much larger courses. This is ideal for small Heroku accounts or web servers that support multiple applications.

The worker mode is an opt-in upgrade. UDOIT can be run in classic mode w/o any modifications by your server admin, but it will continue to be limited in the same way prior versions were limited.

Limitations present in Classic mode (and all versions prior to 2.3.0):

  • NGINX/Apache timeouts can easily be exceeded before a course scan finishes.
  • Load Balancers timeouts can easily be exceeded before a course scan finishes.
  • PHP.ini max_execution_time can easily be exceeded before a course scan finishes.
  • A single user consumes an entire Heroku dyno during a scan.

Changes in this PR:

  • Adds background worker mode. (UFixit still needs to be backroundized)
  • Splits up scanning into multiple "jobs"
  • Simplifies scanning and api code
  • Adds composer's autoloading for classes in lib/
  • Cleans up some of the request vars for process.php
  • Separates tasks for starting a scan, checking progress, and loading results
  • Adds some early support for sqlite (mostly for testing)
  • Adds a lot more unit tests
  • Adds lint scripts to make sure all php compiles
  • Adds phpCS code sniffing and code standard scripts
  • Removes fractured pagination api code
  • Adds php 7.0 and 7.1 to the supported list and Travis tests
  • clean up canvas_api_domain to prevent multiple trailing slashes
  • update logo to use svg
  • moves key in api requests from get params into request header
  • adds logging of each api request via monolog's addInfo()
  • add user id checking to process.php to make sure jobs aren't started without the needed vars
  • change process.php errors to use json and display them in page
  • adds UDOIT_VERSION and displays it in the header
  • now ensures the report shown to the user is sorted in a specific order
  • docker setup updated to aid testing in multiple php versions
  • adds another table for job queue (and matures the migrations slightly)
  • adds a background worker for new heroku installs
  • opens videos found in module_urls in a new window (the canvas links to them stopped working, or never did)

Fixes #279

@iturgeon iturgeon requested a review from bagofarms June 7, 2017 18:57
@iturgeon
Copy link
Member Author

iturgeon commented Jun 7, 2017

This isn't ready to merge yet, just opened it up while I work through a few remaining issues.

@iturgeon iturgeon modified the milestone: v2.2.0 Jun 8, 2017
@iturgeon iturgeon changed the base branch from master to dev/v2.2.0 June 9, 2017 23:12
* adds composer scripts for `lint`, `test`, `start`
* adds background worker to Heroku Proc file
* set default tests to skip functional tests
* adds new table - job_queue for holding jobs for the worker
* adds support for sqlite (mostly for fast unit testing)
* adds mockery and vfsstream for testing
* config options for workers added
* reorganized configs and setting files
* added config for tests
* changed config/tests.php to only be loaded as needed
* added UdoitDB database abstraction class that handles reconnecting for long running jobs
* added UdoitJob class that manages running and creating queued jobs
* expanded UdoitUtils to centeralize more functions of the application
* added worker that processes jobs in the background
* added a ton of unit tests
* updated default.js to accomidate changes to background workers
* updated a lot of rel require/include references to use absolute refs
* heavily simplified index.php to act purely as an LTI launch target
* simplified oauth2responce.php
* updated parseResults.php to use GET rather then POST
* changed process.php to simply store the jobs to do
* changed progress.php to simply report on the status of jobs
* added missing data from the sample report.json
* moved the code used to display the scanner from index.php to a new file scanner.php
* all .sh scripts moved to composer scripts
* Adopted composer autoloading for all classes
@iturgeon iturgeon force-pushed the issue/279-heroku-background-worker branch from 6d8246d to d7af371 Compare June 10, 2017 05:03
@iturgeon iturgeon force-pushed the issue/279-heroku-background-worker branch from e4d8158 to 93c10f9 Compare June 11, 2017 03:06
adds php CodeSniffer to dev requirements
adds symfony 2 code standards to dev requirements
adds custom phpcs rules
applies all fixes required to pass errors for new phpcs rules
removes file system mocks not needed any longer
adds scripts for sniffing
@iturgeon iturgeon force-pushed the issue/279-heroku-background-worker branch from 93c10f9 to 1f673c3 Compare June 11, 2017 03:09
@iturgeon
Copy link
Member Author

This ended up being pretty huge and pushing the codebase forward quite a bit. I updated the pull request overview above.

@bagofarms bagofarms changed the base branch from dev/v2.2.0 to master June 26, 2017 17:26
Copy link
Member

@bagofarms bagofarms left a comment

Choose a reason for hiding this comment

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

I'm reviewing the PR now, but please merge in dev/v2.3.0 and resolve any conflicts.

@bagofarms bagofarms changed the base branch from master to dev/v2.3.0 November 8, 2017 22:38
@bagofarms bagofarms added this to the v2.3.0 milestone Nov 8, 2017
@bagofarms
Copy link
Member

Additionally, Heroku doesn't seem to see the app.json, meaning the database never gets built. Judging by the build log, the composer script gets run, installing all of the dependencies, but the database never gets set up. I've added you to my Heroku instance so you can take a look.

@iturgeon
Copy link
Member Author

iturgeon commented Nov 9, 2017

I'm working on resolving the conflicts.

Looks like I need to make a migration script to move to the worker version

@iturgeon
Copy link
Member Author

Migration added for the job_queue table and fixed all the problems from the merge. We should be able to get back to testing

@bagofarms
Copy link
Member

Detecting YouTube videos without captions doesn't work in Module URLs.
Intermittent error where it only scans module_urls and unscannable, producing zero errors and zero suggestions. The JSON looks like this in the database:
{"total_results":{"errors":0,"warnings":0,"suggestions":0},"content":{"module_urls":{"title":"module_urls","items":[],"amount":0,"time":0},"unscannable":{"title":"unscannable","items":[],"amount":0}},"course":null}

* adds finished log
* silence the worker sleeping msg
* clean up canvas_api_domain to prevent multiple trailing slashes
* fix module_urls api request
* fix modules api url
* update logo svg
* fixes module_urls counts
* moves api key into request header
* adds finalize log report logging
* add user id checking to process
* change error to use json
@iturgeon
Copy link
Member Author

Alright, that last commit contained a bunch of fixes for module_urls and other things.

I updated the bulleted list of features in the pull PR.

I think this is buttoned up at this point. Recommend final testing 🎉

@bagofarms
Copy link
Member

I'm unable to complete a scan, receiving this error: Warning: Invalid argument supplied for foreach() in /app/lib/UdoitUtils.php on line 255

During the scan, announcements, assignments, etc. stacked up in the scan button without ever seeming to complete.

I updated the composer.lock file since there were updates to the composer.json that heroku wasn't taking into account, but that didn't fix the issue.

bagofarms and others added 5 commits November 16, 2017 14:16
the conversion to using a background worker
had a bunch of cleanup to the scanning code.
It incorrectly dealt with files and syllabus.
Thats all fixed now.

Files were trying to use the auth header in the api
requests, but they come with a temporary auth.

And syllabus api was incorrectly assuming it had
to loop of the results, which it doesnt.
@iturgeon
Copy link
Member Author

I was able to fix the files being missed in the scanner. Also found syllabus wasn't scanning and was able to fix up the composer.json/lock files so composer update will work again.

iturgeon and others added 8 commits November 16, 2017 22:08
Just changing how we store the license file in the repository to work better with Github's tools
…cense

Changes license docs to conform to github's practices
renames and changes default value for worker_sleep_seconds
adds a process to expire old tasks
makes sure course, course_id, and user_id are set on report
make sure course data gets set on the report
suppress pdf warnings
fix pdf progress checker
@iturgeon iturgeon force-pushed the issue/279-heroku-background-worker branch from e94d6e5 to 765b3ca Compare November 18, 2017 05:31
@bagofarms bagofarms merged commit a62f7c9 into ucfopen:dev/v2.3.0 Dec 8, 2017
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.

2 participants