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

replace periodic job scheduler with one that is maintained #1756

Merged
merged 2 commits into from
Sep 4, 2018

Conversation

robbkidd
Copy link
Contributor

@robbkidd robbkidd commented Aug 29, 2018

sidetiq was heretofore our background job scheduler. It was deprecated a couple years ago, but still worked for Supermarket's purposes.

sidetiq required the use of celluloid which has also been deprecated. The version of celluloid require was old and required the use of a celluloid subcomponent that was brought in via git submodules.

There is much sadness in everything above this line.

sidekiq-cron has been a go-to replacement for sidetiq in other projects since its maintainer declared it unmaintained. Here, we replace sidetiq with sidekiq-cron. This required:

  • updated the Gemfile
  • removing the require shenanigans for celluloid in application.rb
  • setting the schedule for Supermarket's recurring jobs in the Sidekiq
    startup config in the initializer; putting in Sidekiq's startup
    prevents it from being called and attempting a write to Redis whenever
    the Rails app is initialized (e.g. precompiling assets, running tests,
    opening the console).
  • removing the Sidetiq pieces from the scheduled jobs

The Jobs:

SitemapRefreshWorker was simple; remove Sidetiq things.

OauthTokenRefreshScheduleWorker was more invasive. sidekiq-cron does not have the same schedule backfill feature, so there is no last_occurence or current_occurence to work with. This job now assumes that it is run every five minutes to refresh any tokens. The querying of account with access tokens expiring soon moves to the Account class. This query gets to define what "soon" means. The job itself becomes much simpler by relying on this query and scheduling the token refresh for any account returned.

Fixes #1754

@robbkidd robbkidd requested a review from a team August 29, 2018 22:08
@robbkidd robbkidd requested a review from a team as a code owner August 30, 2018 00:14
@robbkidd
Copy link
Contributor Author

Some issues:

⏲ The previous sidetiq implementation of the scheduled oauth2 token refresh job passed in times for the lower and upper time bounds for the query. This PR's current implementation of that job uses Time.zone.now and some rounding, so the specs for the job fail intermittently because of Wibbly Wobbly Timey Wimey reasons. Need to think about other ways to implement this refresh with sidekiq-cron and a repeatable Time.

😭 The scheduling of jobs as written at the moment (in the Rails initializer) breaks precompiling assets on hosts that don't have Redis running (like, say, our we-don't-run-services-here builder hosts that crank out the release). The initializer blindly tries to insert the jobs into the schedule in all environments at anytime the Rails application is initialized (e.g. precompiling assets, running specs, opening the rails console). This seems less than ideal.

♻️ Any cleanup necessary of Redis keys left behind by Sidetiq?

@robbkidd robbkidd force-pushed the robb/replace-sidetiq-with-sidekiq-cron branch from 37c4eef to d287bae Compare August 30, 2018 17:43
@robbkidd
Copy link
Contributor Author

⏲✅Moved the time-sensitive query to a query method on Account that takes a time to build the query's constraint around. Made repeatable testing possible again and simplified the code by putting concerns in more appropriate places.

😭➡️😂Moved the scheduling of jobs into Sidekiq's configure_server callback so that the scheduling of jobs (and therefore writes to Redis) are only attempted at a time that it would be reasonable to assume Redis is reachable.

@robbkidd robbkidd force-pushed the robb/replace-sidetiq-with-sidekiq-cron branch 2 times, most recently from 9560a1d to 6a952ea Compare August 30, 2018 21:00
@robbkidd
Copy link
Contributor Author

This is green. I'll put on the backlog to maybe add a Rake task to clean up old sidetiq Redis keys, but it doesn't hurt anything to leave them.

READY FOR REVIEW 🎉

@robbkidd robbkidd changed the title 🚧 WIP: replace scheduled job processor replace periodic job scheduler Aug 30, 2018
@robbkidd
Copy link
Contributor Author

@pwelch @rmoriz 👀

@robbkidd
Copy link
Contributor Author

robbkidd commented Aug 30, 2018

@rmoriz I'll point out that I cherry-picked your commit with the rubyzip CVE exclusion. (Actually, I plucked that out to a separate PR #1759.) Thank you for that.

robbkidd and others added 2 commits August 30, 2018 17:56
Signed-off-by: Robb Kidd <rkidd@chef.io>
sidetiq[1] was heretofore our periodic background job scheduler. It was
deprecated a couple years ago, but still worked for Supermarket's
purposes.

sidetiq required the use of celluloid which has also been deprecated.
The version of celluloid require was old and required the use of a
celluloid subcomponent that was brought in via git submodules.

There is much sadness in everything above this line.

sidekiq-cron[2] has been a go-to replacement for sidetiq in other projects
since its maintainer declared it unmaintained. Here, we replace sidetiq
with sidekiq-cron. This required:

* updated the Gemfile
* removing the require shenanigans for celluloid in application.rb
* setting the schedule for Supermarket's recurring jobs in the Sidekiq
  startup config in the initializer; putting in Sidekiq's startup
  prevents it from being called and attempting a write to Redis whenever
  the Rails app is initialized (e.g. precompiling assets, running tests,
  opening the console).
* removing the Sidetiq pieces from the scheduled jobs

The Jobs:

SitemapRefreshWorker was simple; remove Sidetiq things.

OauthTokenRefreshScheduleWorker was more invasive. sidekiq-cron does not
have the same schedule backfill feature, so there is no last_occurence
or current_occurence to work with. This job now assumes that it is run
every five minutes to refresh any tokens. The querying of account with
access tokens expiring soon moves to the Account class. This query gets
to define what "soon" means. The job itself becomes much simpler by
relying on this query and scheduling the token refresh for any account
returned.

[1] https://github.com/endofunky/sidetiq
[2] https://github.com/ondrejbartas/sidekiq-cron

Co-Authored-by: Paul Welch <pwelch@chef.io>
Signed-off-by: Robb Kidd <rkidd@chef.io>
@robbkidd robbkidd force-pushed the robb/replace-sidetiq-with-sidekiq-cron branch from 6a952ea to bc9f1f5 Compare August 30, 2018 21:57
@robbkidd robbkidd changed the title replace periodic job scheduler replace periodic job scheduler with one that is maintained Aug 30, 2018
@rmoriz
Copy link
Contributor

rmoriz commented Aug 31, 2018

Thanks! please ping me when ready so I can rebase #1753

Copy link
Contributor

@pwelch pwelch left a comment

Choose a reason for hiding this comment

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

:shipit:

@rmoriz
Copy link
Contributor

rmoriz commented Sep 4, 2018

ping? @robbkidd

@robbkidd
Copy link
Contributor Author

robbkidd commented Sep 4, 2018

It's baked successfully over the weekend in my test environment. Mergin' for integration testing in Staging.

@robbkidd robbkidd merged commit 6fec125 into master Sep 4, 2018
@robbkidd robbkidd deleted the robb/replace-sidetiq-with-sidekiq-cron branch September 14, 2018 21:06
@tas50 tas50 added Type: Bug Does not work as expected. and removed Bug labels Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Does not work as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants