This is marked as experimental, but shouldn't stay that way forever. We should target 13.0 (2020-05-22) to un-mark this as such, but if it's not ready by that point, we shouldn't force it. To do that, we'd want to:
Rails
Support both --experimental-queue-selector and --queue-selector in the CLI.
Create a follow-up issue to remove support for the 'experimental' variants of these arguments / config options (in 1 and 2) in the next yearly release. So if we do this in 13.0, the next issue should target 14.0.
All of this assumes that this experimental syntax doesn't undergo significant changes between now and then; if it does, then we should obviously rethink
✓
10 of 10 checklist items completed
· Edited by
Sean McGivern
This seems like an interesting and useful feature. However, it would be far more useful if it were possible to set the concurrency for individual queues, rather than relying only on existing min/max settings.
For example, to optimize performance of CPU-bound queues, I would want to create a queue group with those queues (IE resource_boundary=cpu) and set their concurrency equal to the number of CPU cores on my machine. However, for queues which are not CPU-bound, I want to set concurrency to a value which is higher than the number of CPU cores that I have.
So, in short, I would probably only use this feature if it were released in conjunction with a change that allowed the control of concurrency for individual queue groups (whether using queue-selector or not).
As a proposal, perhaps this configuration settings could (alternatively) be a hash, where the key is the queue name(s) or selector(s) and the value is the level concurrency desired.
When we're deploying GitLab.com, we use entirely separate nodes for this: so we have one node just processing resource_boundary=cpu jobs, for instance (and that node can have more CPU available). This means that we're not currently feeling the issue you have, because we can set concurrency for the node.
However, I think this proposal makes sense. Anything that can be applied to an individual Sidekiq process can, in principle, be configurable per-queue-group. At the moment the only really practical settings for this are the concurrency ones, as you say. (You could technically set different Rails environments too, but that would be weird and confusing.)
The only problem then would be how to translate this into CLI arguments, as that's what the configuration in gitlab.rb boils down to. I think the easiest solution is probably to require --min-concurrency and --max-concurrency to either:
Have length 1.
Have length equal to the length of the queue groups, when split with spaces.
So we could have:
# Concurrency of 5 for all processes$ sidekiq-cluster --min-concurrency=5 --max-concurrency=5 --experimental-queue-selector'resource_boundary=cpu''resource_boundary!=cpu'# Concurrency of 5 for CPU-bound, concurrency of 10 for non-CPU-bound$ sidekiq-cluster --min-concurrency='5 10'--max-concurrency='5 10'--experimental-queue-selector'resource_boundary=cpu''resource_boundary!=cpu'
@oswaldo what do you think? If this makes sense we should probably extract it to a separate issue.
For example, to optimize performance of CPU-bound queues, I would want to create a queue group with those queues (IE resource_boundary=cpu) and set their concurrency equal to the number of CPU cores on my machine. However, for queues which are not CPU-bound, I want to set concurrency to a value which is higher than the number of CPU cores that I have.
The proposal makes sense! Though there's a caveat on the assumption that Sidekiq would use all CPU cores in parallel if the concurrency is set as equal to CPU cores given how GIL works (see https://medium.com/@adrianbooth/exploring-rubys-multi-threaded-weirdness-d5dc61883591), though, that would be useful for workers with external_dependencies (given possible external IO).
what do you think? If this makes sense we should probably extract it to a separate issue.
@smcgivern I like it! We might want to tweak the CLI in order to fail if the amount of given concurrencies doesn't match with the queue amount / queue-group, or simply fallback to the default concurrency.
@smcgivern Do you think we will still unmark this as "experimental" if we start the implementation of &194, perhaps we'll need an issue there to remove the selector out of the sidekiq-cluster CLI when it is only used by the application?
@smcgivern with &194 on hold should this be reconsidered? Some issues I see:
It's only EE at the moment
Without this feature it is not possible to setup a queue to handle all default queues not covered by other queue groups like. At minimum I think this should be in CE.
@bbodenmiller good call, thanks for the ping! It's actually not EE-only at the moment; the documentation is wrong and I'll fix that. Did you have issues using it on a CE install?
@rnienaber I'm going to take this now as it's been stable for a while and we don't plan any dramatic changes. I'll make it available as both queue_selector and experimental_queue_selector, then set an item for 14.0 (probably in &198?) to remove the experimental variant.
It won't be my top priority but I'll get it started.
Sean McGivernmarked the checklist item Create a follow-up issue to remove support for the 'experimental' variants of these arguments / config options (in 1 and 2) in the next yearly release. So if we do this in 13.0, the next issue should target 14.0. as completed
marked the checklist item Create a follow-up issue to remove support for the 'experimental' variants of these arguments / config options (in 1 and 2) in the next yearly release. So if we do this in 13.0, the next issue should target 14.0. as completed
Sean McGivernchanged the descriptionCompare with previous version
marked the checklist item Add this to the default gitlab.rb template, as this was left out of gitlab-org/omnibus-gitlab!3920 (merged) because it's experimental as completed
Sean McGivernmarked the checklist item Update the CNG build images to support either (no need to check for both here). as completed
marked the checklist item Update the CNG build images to support either (no need to check for both here). as completed
@joshlambert sorry, I know this is late in the cycle. I added a checkbox item to add this to the release post above but haven't written that yet, and I see that we're past the due date for adding content blocks (gitlab-com/www-gitlab-com!66652 (merged)).
Should we try to squeeze this in or should we just leave it out of the release post?
@joshlambert I was trying to write a release post item for this, but I wasn't sure if it was a deprecation or a feature (or both!). I was going to write something like this for a deprecation:
GitLab contains a large number of background job queues. Some administrators may want to have multiple background job processes, each running different workloads.
Previously, we only supported specifying the queues handled for a particular process by name, or using an experimental option to allow selecting queues by attributes.
This option - previously experimental_queue_selector - is now marked as available under queue_selector. experimental_queue_selector will continue to work until GitLab 14.0.
This option - previously experimental_queue_selector - is now marked as available under queue_selector. experimental_queue_selector will continue to work until GitLab 14.0.
Maybe rephrase slightly do:
This option - previously experimental_queue_selector - is no longer experimental and has been renamed to [queue_selector](https://docs.gitlab.com/ee/administration/operations/extra_sidekiq_processes.html#queue-selector). experimental_queue_selector will continue to work [until GitLab 14.0]().
With a link to the deprecation item anchor.
By the way we are past the normal process for %13.6, but can include directly when working with @jheimbuck_gl