Workers that cannot be easily migrated to one-queue-per-shard

In &469, we're moving from a one-worker-one-queue model to many-workers-one-queue. Some workers can't be easily migrated. That means we need to either exclude them from this work or fix them before migrating them to a shared queue.

Check their own queue size

These workers depend on checking their own queue size, which won't work in this model.

Feature Category	Workers	Note
source_code_management	UpdateAllMirrorsWorker / ProjectImportScheduleWorker	UpdateAllMirrorsWorker depends on the queue size for ProjectImportScheduleWorker, which is on catchall (#1064 (closed) / gitlab-org/gitlab#325496 (comment 550866012))
source_code_management	::HashedStorage::RollbackerWorker	catchall but does not run on GitLab.com. Related to Gitlab::HashedStorage::Migrator: gitlab-org/gitlab#340629 (closed)
source_code_management	::HashedStorage::ProjectRollbackWorker	catchall but does not run on GitLab.com. Related to Gitlab::HashedStorage::Migrator: gitlab-org/gitlab#340629 (closed)
	Gitlab::SidekiqQueue	Keep it as it is. We would need to make some noise around this when we make &194 or &447 available to self-managed installations who could also be using this.

Previously: #1064 (closed), #995 (closed)

Jobs written to Redis without passing through the application

• EmailReceiverWorker
• ServiceDeskEmailReceiverWorker

The EmailReceiverWorker and ServiceDeskEmailReceiverWorker rely on our use of mail_room to take emails from an IMAP mailbox and push them into Sidekiq. mail_room does not use our application code directly, and it does not run our Sidekiq client middleware.

We already have these queue names in our application (to pass to mail_room) in Gitlab::MailRoom::ADDRESS_SPECIFIC_CONFIG, so we could use the worker routing there, but that is probably a task of its own rather than one we want to bundle in with other queues: #1263 (closed)

added ServiceSidekiq groupscalability labels

mentioned in issue #1064 (closed)

changed the description

mentioned in epic &447

changed the description

@smcgivern Initially we should exclude them from the &447 epic (which you have already done), but longer-term we need to have them fixed so that there is only one method of using Sidekiq. I'm concerned that if we have some workers in their own queues that new workers might be added with this model.

@reprazent (or @smcgivern) Why are these workers checking the queue length? What do they do with the length information? I'm not looking for a detailed description of all of them, just an idea of why one might need this.

@rnienaber typically they want to do some kind of rate control or throttling, so that we don't have too many expensive workers running at the same time.

For instance, the hashed storage stuff uses it to prevent any concurrency: if you try to schedule some hashed storage migrations while some are in progress, it will not be permitted.

@smcgivern As Quang-Minh pointed out to me in #995 (comment 546225526) the elastic workers mentioned in the description don't use ApplicationWorker#queue_size. But rather Elastic:::IndexingControlService#queue_size.

I confused myself with this. We're not initialising a worker, but we're initialising a IndexingControlService with a worker-class. Those "queue sizes" are a reflection of elements in a redis zset. The keys for those set are based on the worker's class name: https://gitlab.com/gitlab-org/gitlab/blob/4e580ca447fe575469ce25610980353a5d1c83d2/ee/app/services/elastic/indexing_control_service.rb#L15

So I think we're good on those workers. I'll remove them from the description. Please re-add them if I misread that code again .

@reprazent oh right, I even remember we talked to the search team about those manual job sets they were managing

So this issue is basically:

1. Two queues that never run on GitLab.com.
2. One that needs attention in general.
3. Some stuff we wrote.

changed the description

mentioned in issue #538

added workflow-infraTriage label

added to epic &469

marked this issue as related to #1027 (moved)

marked this issue as related to #1028 (closed)

changed title from Workers that depend on checking their own queue size to Workers that cannot be easily migrated to one-queue-per-shard

changed the description

mentioned in issue #1136 (closed)

mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!988 (merged)

mentioned in issue #1074 (closed)

mentioned in issue #1161 (closed)

changed the description

I created gitlab-org/gitlab!66553 (merged) to tag these. As @cmiskell pointed out in Slack, this simplifies the routing rules because we can just put this at the top:

["tags=needs_own_queue", nil]

It doesn't simplify the shard queue selectors, unfortunately, but that's still a win.

mentioned in merge request gitlab-org/gitlab!66553 (merged)

mentioned in issue production#5212

In gitlab-org/gitlab!66553 (merged), Heinrich pointed out that in the migration doc, we are guiding the users to check for the queue length of BackgroundMigrationWorker beforehand. It looks like the queue-size checking method is recommended for GitLab < 12.8. Since GitLab 12.9, we have can use Gitlab::BackgroundMigration.remaining, which is more accurate. Before this version, Sidekiq routing rule feature is not available either.

However, the doc is not super clear, and a little confusing if the users use both method. We would need to emphasize the versioning difference a little bit.

Just a note that Gitlab::BackgroundMigration.remaining also checks the Sidekiq queue size: https://gitlab.com/gitlab-org/gitlab/blob/03310908ffb5269f91b6f46e475efd792ddf459c/lib/gitlab/background_migration.rb#L62-68

It does, but it also iterates over the entire scheduled set (which is shared between queues). We could do the same for whatever the actual queue used by BackgroundMigrationWorker is, which would then mean background migrations wouldn't need their own queue I think.

@engwan @qmnguyen0711 is that right? If it is then I can create an MR to let Gitlab::BackgroundMigration.remaining use that method, and then we can say background migrations can share a queue. I'll also update the docs to remove the pre-12.9 method of finding this out.

Yeah, that could work. So we iterate both the scheduled set and the queue.

It does, but it also iterates over the entire scheduled set (which is shared between queues). We could do the same for whatever the actual queue used by BackgroundMigrationWorker is, which would then mean background migrations wouldn't need their own queue I think.

Agree. I think this method is definitely better, as it does not depend on the queue. Iterating through the whole scheduled set and queue seems to be expensive, but I think that's properly okay in this case. We don't run this command in a daily basis though.

I've created gitlab-org/gitlab!67432 (merged) for this now.

mentioned in issue #1215 (closed)

mentioned in epic &148

mentioned in issue #1263 (closed)

changed the description

@smcgivern What is the current plan for this?

@rnienaber I'll assign to myself and take it out of the epic. I need to make sure we've cross-referenced the correct issues in the codebase, and probably we need to document this case, but we have all the problem workers captured here.

I created gitlab-org/gitlab!70069 (merged) to update the code comments to point to the correct issues (all are linked to this issue, too) and document this for self-managed instances.

mentioned in epic &469

mentioned in commit gitlab-com/gl-infra/k8s-workloads/gitlab-com@3efc5f53

mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!1217 (merged)

added ~15973982 workflow-infraIn Progress labels and removed workflow-infraTriage label

removed from epic &469

assigned to @smcgivern

changed the description

marked this issue as related to gitlab-org/gitlab#340629 (closed)

marked this issue as related to #1263 (closed)

Gitlab::SidekiqQueue

@reprazent do you remember what this note was about? I think this is just used by https://docs.gitlab.com/ee/api/admin_sidekiq_queues.html#sidekiq-queues-administration-api, so if we add a worker argument to the API and the class itself, I think we're good?

Ah, found it: #995 (comment 546319401)

Gitlab::SidekiqQueue. AFAIK, this class is used by admin users (us in precise) to drop the jobs from a particular queue. It's still working, but as the queue will mean a totally different thing, we should either implement a replacement. This should be mentioned in #997 (closed) as well.

I think we can keep this API and just add a worker parameter, I'll create an MR for that.

I created gitlab-org/gitlab!70179 (merged) for this. Once that's done, I can close this issue.

mentioned in merge request gitlab-org/gitlab!70069 (merged)

added workflow-infraUnder Review label and removed workflow-infraIn Progress label

mentioned in merge request gitlab-org/gitlab!70179 (merged)

We now document that these workers cannot be migrated right now: https://docs.gitlab.com/ee/administration/operations/extra_sidekiq_routing.html#workers-that-cannot-be-migrated

(I know the table doesn't work, but it does work in https://gitlab.com/gitlab-org/gitlab/blob/master/doc/administration/operations/extra_sidekiq_routing.md#workers-that-cannot-be-migrated, so I asked in Slack: https://gitlab.slack.com/archives/C16HYA2P5/p1631611092183300)

We also won't migrate them on GitLab.com due to putting them first in the routing rules: https://gitlab.com/gitlab-com/gl-infra/k8s-workloads/gitlab-com/-/blob/3cba7a980ad295752746c9fd3e9ec5ae4c60d5b7/releases/gitlab/values/gprd.yaml.gotmpl#L448-451

closed

added workflow-infraDone label and removed workflow-infraUnder Review label

mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!1232 (merged)

mentioned in issue #1281 (closed)

mentioned in issue #1292 (closed)

mentioned in epic &644