From ddc222db8a020368250631ded1066706264c80f0 Mon Sep 17 00:00:00 2001 From: Gregorius Marco Date: Thu, 22 Sep 2022 16:24:06 +0000 Subject: [PATCH 1/4] Update Sidekiq chart queues based on routingRules Since default routing rules is updated in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/97908, the corresponding Sidekiq can just listen on default and mailers queue. If custom routing rules is set, the SIDEKIQ_QUEUES will be generated based on user's routing rules. Changelog: changed --- .../charts/sidekiq/templates/_helpers.tpl | 28 +++++ .../charts/sidekiq/templates/deployment.yaml | 2 +- spec/configuration/sidekiq_spec.rb | 117 +++++++++++++++++- values.yaml | 6 +- 4 files changed, 145 insertions(+), 8 deletions(-) diff --git a/charts/gitlab/charts/sidekiq/templates/_helpers.tpl b/charts/gitlab/charts/sidekiq/templates/_helpers.tpl index 312b335705..0aa044c2b2 100644 --- a/charts/gitlab/charts/sidekiq/templates/_helpers.tpl +++ b/charts/gitlab/charts/sidekiq/templates/_helpers.tpl @@ -54,4 +54,32 @@ Return the sidekiq-metrics TLS secret name */}} {{- define "sidekiq-metrics.tls.secret" -}} {{- default (printf "%s-sidekiq-metrics-tls" .Release.Name) $.Values.metrics.tls.secretName | quote -}} +{{- end -}} + +{{/* +Generates queues based on Sidekiq routing rules if exists. +Otherwise, returns default queues in .Values.queues. + +Structure of routingRules is checked thoroughly so it does not panic here, allowing user friendly error message +returned by _checkConfig_sidekiq.tpl in the end. +*/}} +{{- define "sidekiq.queues" -}} +{{- $queues := "" -}} +{{- with $.Values.global.appConfig.sidekiq.routingRules -}} +{{- if . -}} + {{- $queuesList := list -}} + {{- if kindIs "slice" . }} + {{- range $_, $rule := . -}} + {{- if and (kindIs "slice" $rule) (eq (len $rule) 2) -}} + {{- $queuesList = append $queuesList (index $rule 1) -}} + {{- end -}} + {{- end -}} + {{- end -}} + {{- $queuesList = append $queuesList "mailers" -}} + {{- $queues = join "," $queuesList -}} +{{- end -}} +{{- else -}} + {{- $queues = $.Values.queues -}} +{{- end -}} +{{- $queues -}} {{- end -}} \ No newline at end of file diff --git a/charts/gitlab/charts/sidekiq/templates/deployment.yaml b/charts/gitlab/charts/sidekiq/templates/deployment.yaml index c43cf5c426..d3e88fda81 100644 --- a/charts/gitlab/charts/sidekiq/templates/deployment.yaml +++ b/charts/gitlab/charts/sidekiq/templates/deployment.yaml @@ -199,7 +199,7 @@ spec: - name: SIDEKIQ_TIMEOUT value: {{ default $timeout .timeout | quote }} - name: SIDEKIQ_QUEUES - value: {{ default $.Values.queues .queues | quote | nospace }} + value: {{ default (include "sidekiq.queues" $) .queues | quote | nospace }} - name: SIDEKIQ_NEGATE_QUEUES value: {{ default $.Values.negateQueues .negateQueues | quote | nospace }} - name: SIDEKIQ_DAEMON_MEMORY_KILLER diff --git a/spec/configuration/sidekiq_spec.rb b/spec/configuration/sidekiq_spec.rb index 5080dc0dc3..37315f652b 100644 --- a/spec/configuration/sidekiq_spec.rb +++ b/spec/configuration/sidekiq_spec.rb @@ -150,11 +150,11 @@ describe 'Sidekiq configuration' do end end - context 'when setting extraEnvFrom' do - def deployment_name(pod) - "Deployment/test-sidekiq-#{pod}-v2" - end + def deployment_name(pod) + "Deployment/test-sidekiq-#{pod}-v2" + end + context 'when setting extraEnvFrom' do context 'when the global value is set' do let(:global_values) do YAML.safe_load(%( @@ -773,4 +773,113 @@ describe 'Sidekiq configuration' do end end end + + context 'Generating SIDEKIQ_QUEUES env var' do + let(:default_values) do + YAML.safe_load(%( + certmanager-issuer: + email: test@example.com + + gitlab: + sidekiq: + pods: + - name: pod-1 + - name: pod-2 + )) + end + + def deep_copy(obj) + Marshal.load(Marshal.dump(obj)) + end + + context 'when queues per pod is set' do + let(:values) do + v = deep_copy(default_values) + v['gitlab']['sidekiq']['pods'][0]['queues'] = 'merge' + v['gitlab']['sidekiq']['pods'][1]['queues'] = 'pull' + v + end + + context 'without routingRules' do + it 'should follow queues per pod' do + template = HelmTemplate.new(values) + expect(template.exit_code).to eq(0) + expect(template.env(deployment_name('pod-1'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'merge' } + ) + expect(template.env(deployment_name('pod-2'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'pull' } + ) + end + end + + context 'with routingRules' do + let(:with_routing_rules_values) do + YAML.safe_load(%( + global: + appConfig: + sidekiq: + routingRules: + - ["query", "queue"] + )).merge(values) + end + + it 'should follow queues per pod' do + template = HelmTemplate.new(with_routing_rules_values) + expect(template.exit_code).to eq(0) + expect(template.env(deployment_name('pod-1'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'merge' } + ) + expect(template.env(deployment_name('pod-2'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'pull' } + ) + end + end + end + + context 'when queues per pod is not set' do + context 'without routingRules' do + it 'has default,mailers as default queues' do + default_template = HelmTemplate.new(default_values) + expect(default_template.env(deployment_name('pod-1'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'default,mailers' } + ) + expect(default_template.env(deployment_name('pod-2'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'default,mailers' } + ) + end + end + + context 'with routingRules' do + let(:values) do + YAML.safe_load(%( + global: + appConfig: + sidekiq: + routingRules: + - ["attribute=a", "queue-a"] + - ["attribute=b", "queue-b"] + )).merge(default_values) + end + + it 'is generated based on routingRules' do + template = HelmTemplate.new(values) + expect(template.env(deployment_name('pod-1'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'queue-a,queue-b,mailers' } + ) + expect(template.env(deployment_name('pod-2'), 'sidekiq')) + .to include( + { 'name' => 'SIDEKIQ_QUEUES', 'value' => 'queue-a,queue-b,mailers' } + ) + end + end + end + end end diff --git a/values.yaml b/values.yaml index 9958790d7c..9a3df3d86e 100644 --- a/values.yaml +++ b/values.yaml @@ -1174,15 +1174,15 @@ gitlab: antiAffinityLabels: matchLabels: app: gitaly + # https://docs.gitlab.com/charts/charts/gitlab/sidekiq + sidekiq: + queues: "default,mailers" ## https://docs.gitlab.com/charts/charts/gitlab/migrations # migrations: # enabled: false ## https://docs.gitlab.com/charts/charts/gitlab/webservice # webservice: # enabled: false - ## https://docs.gitlab.com/charts/charts/gitlab/sidekiq - # sidekiq: - # enabled: false ## https://docs.gitlab.com/charts/charts/gitlab/gitaly # gitaly: ## https://docs.gitlab.com/charts/charts/gitlab/gitlab-shell -- GitLab From bcd28d80f6f911c69cbc540dabaccd8a213a8ae9 Mon Sep 17 00:00:00 2001 From: Gregorius Marco Date: Tue, 11 Oct 2022 13:41:47 +0000 Subject: [PATCH 2/4] Deduplicate mailers queue --- charts/gitlab/charts/sidekiq/templates/_helpers.tpl | 2 +- spec/configuration/sidekiq_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/charts/gitlab/charts/sidekiq/templates/_helpers.tpl b/charts/gitlab/charts/sidekiq/templates/_helpers.tpl index 0aa044c2b2..be29f4fa82 100644 --- a/charts/gitlab/charts/sidekiq/templates/_helpers.tpl +++ b/charts/gitlab/charts/sidekiq/templates/_helpers.tpl @@ -75,7 +75,7 @@ returned by _checkConfig_sidekiq.tpl in the end. {{- end -}} {{- end -}} {{- end -}} - {{- $queuesList = append $queuesList "mailers" -}} + {{- $queuesList = append $queuesList "mailers" | uniq -}} {{- $queues = join "," $queuesList -}} {{- end -}} {{- else -}} diff --git a/spec/configuration/sidekiq_spec.rb b/spec/configuration/sidekiq_spec.rb index 37315f652b..72723c91dd 100644 --- a/spec/configuration/sidekiq_spec.rb +++ b/spec/configuration/sidekiq_spec.rb @@ -766,7 +766,6 @@ describe 'Sidekiq configuration' do t = HelmTemplate.new(default_values.deep_merge(chart_values)) expect(t.dig('Deployment/test-sidekiq-pod-1-v2', 'spec', 'template', 'spec', 'terminationGracePeriodSeconds')).to eq(66) end - it 'sets user specified deployment-global value for terminationGracePeriodSeconds in the Pod spec where pod-local value is not set' do t = HelmTemplate.new(default_values.deep_merge(chart_values)) expect(t.dig('Deployment/test-sidekiq-pod-2-v2', 'spec', 'template', 'spec', 'terminationGracePeriodSeconds')).to eq(77) @@ -865,10 +864,11 @@ describe 'Sidekiq configuration' do routingRules: - ["attribute=a", "queue-a"] - ["attribute=b", "queue-b"] + - ["attribute=mailers", "mailers"] )).merge(default_values) end - it 'is generated based on routingRules' do + it 'is generated based on routingRules and deduplicates mailers queue' do template = HelmTemplate.new(values) expect(template.env(deployment_name('pod-1'), 'sidekiq')) .to include( -- GitLab From 4bd8cdf9de674052d274090ef3270c4fc9c1e794 Mon Sep 17 00:00:00 2001 From: Gregorius Marco Date: Fri, 14 Oct 2022 06:13:47 +0000 Subject: [PATCH 3/4] Move default queues to Sidekiq chart values.yaml --- charts/gitlab/charts/sidekiq/values.yaml | 2 ++ values.yaml | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/charts/gitlab/charts/sidekiq/values.yaml b/charts/gitlab/charts/sidekiq/values.yaml index 41875d47e0..4760d29089 100644 --- a/charts/gitlab/charts/sidekiq/values.yaml +++ b/charts/gitlab/charts/sidekiq/values.yaml @@ -25,6 +25,8 @@ tolerations: [] enabled: true queueSelector: false +queues: "default,mailers" + annotations: {} podLabels: {} common: diff --git a/values.yaml b/values.yaml index 9a3df3d86e..840085c3b3 100644 --- a/values.yaml +++ b/values.yaml @@ -1175,8 +1175,6 @@ gitlab: matchLabels: app: gitaly # https://docs.gitlab.com/charts/charts/gitlab/sidekiq - sidekiq: - queues: "default,mailers" ## https://docs.gitlab.com/charts/charts/gitlab/migrations # migrations: # enabled: false -- GitLab From 33c18ea55853b5e78e73653e1cb992a6af2c77e9 Mon Sep 17 00:00:00 2001 From: Gregorius Marco Date: Wed, 26 Oct 2022 08:29:34 +0000 Subject: [PATCH 4/4] Revert changes to root values.yaml --- values.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/values.yaml b/values.yaml index 840085c3b3..9958790d7c 100644 --- a/values.yaml +++ b/values.yaml @@ -1174,13 +1174,15 @@ gitlab: antiAffinityLabels: matchLabels: app: gitaly - # https://docs.gitlab.com/charts/charts/gitlab/sidekiq ## https://docs.gitlab.com/charts/charts/gitlab/migrations # migrations: # enabled: false ## https://docs.gitlab.com/charts/charts/gitlab/webservice # webservice: # enabled: false + ## https://docs.gitlab.com/charts/charts/gitlab/sidekiq + # sidekiq: + # enabled: false ## https://docs.gitlab.com/charts/charts/gitlab/gitaly # gitaly: ## https://docs.gitlab.com/charts/charts/gitlab/gitlab-shell -- GitLab