Skip to content

Extract sidekiq-cluster to top-level directory

Matthias Käppler requested to merge 344794-sidekiq-cluster-extraction into master

What does this MR do and why?

While working on #343759 (closed), we ran into LOAD_PATH issues where due to sidekiq-cluster redefining some dependencies in lib/, and code in lib/ being auto-loaded in sidekiq workers, we ended up with duplicate definitions of some classes such as Settings.

After some discussion, we decided that the cleanest approach would be to extract all sidekiq-cluster related code into its own top-level directory, because:

  • It addresses the immediate problem of not putting sidekiq-cluster specific code on LOAD_PATH automatically
  • With the ongoing work in &6409 (closed), it is becoming a more self-contained application with added complexity, so it makes sense to live in its own tree
  • It was never really library code to begin with, and it was sitting in lib/ mostly for convenience

How to set up and validate locally

  1. Run sidekiq-cluster
  • GDK: gdk start rails-background-jobs
  • GCK: make sidekiq
  1. Everything should operate normally

Omnibus

Tested with an image built from this branch (registry.gitlab.com/gitlab-org/build/omnibus-gitlab-mirror/gitlab-ee:1f38e88ad859b96636ed58e2f249f2ed5573300e), everything seems to be working correctly:

root@local:/# gitlab-ctl status sidekiq
run: sidekiq: (pid 556) 1339s; run: log: (pid 570) 1338s

Screenshot_from_2021-11-03_14-02-27

Escaping lint checks?

One thing I was wondering about was whether moving code out of lib/ would make it escape static analysis or other jobs that expect all Ruby code to sit in app/, lib/ or spec/.

I checked RuboCop, and we use a deny-list approach to which files should be checked, so it should automatically be included.

I am uncertain about other checks we might be performing.

UPDATE:

  • We updated test_level definitions to map the new spec/commands folder to integration tests, which is where the cli_spec for sidekiq-cluster now lives. This was just a clean-up and not really specific to changes in this MR.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #344794 (closed)

Edited by Matthias Käppler

Merge request reports