[Sidekiq] Support for podAntiAffinity with match labels of deployment name
What does this MR do?
Implement podAntiAffinity in such way that allows to spread out Sidekiq pods across the cluster.
Based on @WarheadsSE 's suggestion - adding queue-pod-name
to the list of matchLabels
in podAntiAffinity
section should provide some immediate relief as it should prevent 2 pods with the same queue spread out to different nodes.
Related issues
Closes #2857 (closed)
Checklist
See Definition of done.
For anything in this list which will not be completed, please provide a reason in the MR discussion.
Required
-
Merge Request Title and Description are up to date, accurate, and descriptive -
MR targeting the appropriate branch -
MR has a green pipeline on GitLab.com
Expected (please provide an explanation if not completing)
-
Test plan indicating conditions for success has been posted and passes -
Documentation created/updated -
Tests added -
Integration tests added to GitLab QA -
Equivalent MR/issue for omnibus-gitlab opened
Merge request reports
Activity
changed milestone to %14.4
assigned to @dmakovey
1 Warning ⚠ Please check the QA job and compare with builds on master. If no new failures are reported in QA job, add 'QA:verified' label. 1 Message 📖 Please add the workflowready for review label once you think the MR is ready to for an initial review. If from a community member, ask that the Community contribution label be added as well.
Merge requests are handled according to the workflow documented in our handbook and should receive a response within the limit documented in our First-response SLO.
If you don't receive a response, please mention
@gitlab-org/distribution
, or one of our Project MaintainersGenerated by
🚫 Dangeradded workflowin dev label
added 1 commit
- 4d19f874 - implement simple anti-affinity rule based on queue name
- Resolved by Dmytro Makovey
will need to test it out on various-sized clusters
added workflowready for review label and removed workflowin dev label
Testing
KinD configuration
I modified
examples/kind/kind-ssl
to provision 2 worker nodes so the workloads can be spread out base don the antiAffinity:diff --git a/examples/kind/kind-ssl.yaml b/examples/kind/kind-ssl.yaml index ef2a4993f..3c4c8d52d 100644 --- a/examples/kind/kind-ssl.yaml +++ b/examples/kind/kind-ssl.yaml @@ -16,3 +16,5 @@ nodes: - containerPort: 32022 hostPort: 32022 listenAddress: "0.0.0.0" +- role: worker +- role: worker
$ kind create cluster --config examples/kind/kind-ssl.yaml
Values
gitlab: sidekiq: antiAffinity: hard minReplicas: 2 pods: - name: "cronjobs" queues: "cronjob:*" - name: catch-all queues: "*" global: antiAffinity: hard
Node separation per Sidekiq Deployment/Queue
$ kubectl get pods -lapp=sidekiq -owide | awk '{print $1,$7}' NAME NODE gitlab-sidekiq-catch-all-v1-84475bd796-7rlz7 kind-worker2 gitlab-sidekiq-catch-all-v1-84475bd796-85gdk kind-worker gitlab-sidekiq-cronjobs-v1-7cfd448489-hx6tc kind-worker2 gitlab-sidekiq-cronjobs-v1-7cfd448489-mjxqh kind-worker
Note that each Sidekiq Deployment/Queue's pods are spread out between the 2 available nodes.
Comparison to Webservice
It's worth noting that we do something similar already with Webservice when there is more than one Deployment:
podAntiAffinity: requiredDuringSchedulingIgnoredDuringExecution: - topologyKey: "kubernetes.io/hostname" labelSelector: matchLabels: app: webservice release: test gitlab.com/webservice-name: default # <- already injects Deployment-specific label
Edited by Mitchell NielsenThanks @dmakovey! Approved and up for maintainer review.
requested review from @twk3
added workflowin review label and removed workflowready for review label
requested review from @mnielsen
- Resolved by Mitchell Nielsen
My only concern with this is confirmation of impact in an upgrade process. We need to know that a) upgrading to this functions b) "outages" are not incurred or are very quickly resolved.
changed milestone to %14.5
added missed-deliverable missed:14.4 labels
removed review request for @twk3
@WarheadsSE, @mnielsen : based on !2227 (comment 711759199) our "transition" option is to use
Recreate
strategy. After which everything settles in and further updates do not requireRecreate
. So my question is: should we advise in release notes about this as a transitional step for those who implementhard
antiaffinity rather than codifying that change as a permanent solution?Edited by Dmytro MakoveyNote on bumped pods per label change:
$ k get pods Default context: gke-dmitry-olm in gke_cloud-native-182609_us-central1-b_dmitry-olm cluster Default namespace: gitlab NAME READY STATUS RESTARTS AGE ... gitlab-issuer-4-85rb6 1/1 Running 0 2m6s gitlab-migrations-4-5pmwp 0/1 Completed 0 2m6s ... gitlab-minio-create-buckets-4-pflp7 0/1 Completed 0 2m6s ... gitlab-sidekiq-catch-all-v1-5d4c68dc5c-kprw5 1/1 Running 0 2m4s gitlab-sidekiq-cronjobs-v1-75475974b4-6zs8r 1/1 Running 0 2m4s gitlab-sidekiq-github-v1-5677f66749-tcst7 1/1 Running 0 2m4s ...
in other words it seems we're just dealing with jobs re-running (an OK thing) and relevant pod recreation (expected)
added 38 commits
-
c95f57e2...dc747e60 - 37 commits from branch
master
- d4d85e1e - implement simple anti-affinity rule based on queue name
-
c95f57e2...dc747e60 - 37 commits from branch
- Resolved by Mitchell Nielsen
Testing
Cluster setup
diff --git a/examples/kind/kind-ssl.yaml b/examples/kind/kind-ssl.yaml index ef2a4993f..3c4c8d52d 100644 --- a/examples/kind/kind-ssl.yaml +++ b/examples/kind/kind-ssl.yaml @@ -16,3 +16,5 @@ nodes: - containerPort: 32022 hostPort: 32022 listenAddress: "0.0.0.0" +- role: worker +- role: worker
kind create cluster --config examples/kind/kind-ssl.yaml
Values
gitlab: sidekiq: antiAffinity: hard minReplicas: 2 pods: # list of queues: https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/workers/all_queues.yml - name: "cronjobs" queues: "cronjob:*" - name: catch-all queues: "*" global: antiAffinity: hard
Results
1. Install from
master
Pods came up as expected, with each queue's second pod stuck in
Pending
:$ k get pods -o json -l app=sidekiq | jq '[.items[] | {name: .metadata.name, labels: .metadata.labels, containerNodes: .spec.nodeName}]' [ { "name": "gitlab-sidekiq-catch-all-v1-7669568687-7t9r7", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.1", "heritage": "Helm", "pod-template-hash": "7669568687", "queue-pod-name": "catch-all", "release": "gitlab" }, "containerNodes": null }, { "name": "gitlab-sidekiq-catch-all-v1-7669568687-szg4q", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.1", "heritage": "Helm", "pod-template-hash": "7669568687", "queue-pod-name": "catch-all", "release": "gitlab" }, "containerNodes": "kind-worker" }, { "name": "gitlab-sidekiq-cronjobs-v1-56bbd567c-9kpql", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.1", "heritage": "Helm", "pod-template-hash": "56bbd567c", "queue-pod-name": "cronjobs", "release": "gitlab" }, "containerNodes": null }, { "name": "gitlab-sidekiq-cronjobs-v1-56bbd567c-kg787", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.1", "heritage": "Helm", "pod-template-hash": "56bbd567c", "queue-pod-name": "cronjobs", "release": "gitlab" }, "containerNodes": "kind-worker2" } ]
2. Install from MR branch
As mentioned,
-v1
Sidekiq pods were immediately terminated.The
-v2
Sidekiq pods came up, with each queue's pods on separate nodes:$ k get pods -o json -l app=sidekiq | jq '[.items[] | {name: .metadata.name, labels: .metadata.labels, containerNodes: .spec.nodeName}]' [ { "name": "gitlab-sidekiq-catch-all-v2-585b57bb7-ltkn7", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.0", "heritage": "Helm", "pod-template-hash": "585b57bb7", "queue-pod-name": "catch-all", "release": "gitlab" }, "containerNodes": "kind-worker" }, { "name": "gitlab-sidekiq-catch-all-v2-585b57bb7-qrmfl", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.0", "heritage": "Helm", "pod-template-hash": "585b57bb7", "queue-pod-name": "catch-all", "release": "gitlab" }, "containerNodes": "kind-worker2" }, { "name": "gitlab-sidekiq-cronjobs-v2-7c95588f8d-d5gt9", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.0", "heritage": "Helm", "pod-template-hash": "7c95588f8d", "queue-pod-name": "cronjobs", "release": "gitlab" }, "containerNodes": "kind-worker2" }, { "name": "gitlab-sidekiq-cronjobs-v2-7c95588f8d-rfz2j", "labels": { "app": "sidekiq", "chart": "sidekiq-5.4.0", "heritage": "Helm", "pod-template-hash": "7c95588f8d", "queue-pod-name": "cronjobs", "release": "gitlab" }, "containerNodes": "kind-worker" } ]
No more pods stuck in Pending
✅ Note that this migration bounced all of the following:
- Gitaly
- Gitlab-Runner
- GitLab-Shell
- TaskRunner
- Webservice
This is likely because this branch needs to be rebased with more recent changes from
master
, but it's worth testing to double-check that.
added 15 commits
-
d4d85e1e...fac2f5c4 - 14 commits from branch
master
- ff1f5923 - Merge branch 'master' of gitlab.com:gitlab-org/charts/gitlab into 2857-support-for-podantiaffinity
-
d4d85e1e...fac2f5c4 - 14 commits from branch
- Resolved by Mitchell Nielsen
Functionally, this MR seems to be in a good state. As such, I'm going to approve and hand up for maintainer review.
Notes for the maintainer: as for implementation, !876 (merged) was a similar change (it added
-v1
), and it had two blockers:- Documentation for the change: !1110 (merged)
- Waiting for the 3.x release
Will it be best to follow a similar path for this as well? My 2 cents: adding the documentation is straightforward, but waiting for the next major release means this MR would be delayed for quite some time. Open to your thoughts on this one based on past experience
👍
requested review from @WarheadsSE
removed review request for @WarheadsSE
Thanks again @dmakovey
👍 Up for maintainer review.
requested review from @WarheadsSE
- Resolved by Dmytro Makovey
enabled an automatic merge when the pipeline for 30914c04 succeeds
mentioned in commit 14ebba0f
mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!1326 (merged)
@dmakovey I noticed the PDB name didn't get incremented with the Deployments and HPA's. Is this desired? We now have a bit of inconsistent naming of objects. I just want to make sure this isn't accidental and/or something we want to look into adjusting prior to the next release.
@WarheadsSE that is precisely why I did not bump PDB since it referenced labels that have no
-v?
suffixes
mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!1330 (merged)
mentioned in merge request gitlab-com/gl-infra/k8s-workloads/gitlab-com!1327 (merged)
added release post item label
mentioned in merge request gitlab-com/www-gitlab-com!93977 (merged)
mentioned in issue gitlab-org/cloud-native/gitlab-operator#451 (closed)
mentioned in issue #2300