Skip to content

Sidekiq: Enforce separate ports for metrics and health checks

Matthias Käppler requested to merge 3198-cleanup-port-default into master

What does this MR do?

We recently merged !2479 (merged), which sets an explicit health check port for Sidekiq. This is good and expected since falling back to the metrics server port is deprecated behavior and removed in 15.0.

We now also enforce this behavior by explicitly running a config check to ensure these ports are distinct. I also removed the enabled toggle for health checks; this is a remnant from using the same set of settings keys as the exporter, but it does not make sense to disable health checks since Sidekiq would never come up. In fact, even if someone disabled this prior, we would still point the kubelet to /readiness, all we would do is not start the server, which doesn't make a whole lot of sense.

Note: we do allow ports to match if metrics are disabled since then only one server will start.

Related issues

Test plan

Set server ports to the same value as the metrics server default port:

helm upgrade -i gitlab gitlab/gitlab \
  --set gitlab.sidekiq.health_checks.port=3807 \
  --set certmanager-issuer.email=me@example.com
Error: UPGRADE FAILED: template: gitlab/templates/NOTES.txt:126:3: executing "gitlab/templates/NOTES.txt" at <include "gitlab.checkConfig" .>: error calling include: template: gitlab/templates/_checkConfig.tpl:101:54: executing "gitlab.checkConfig" at <fail>: error calling fail: 
CONFIGURATION CHECKS:

sidekiq:
    metrics.port and health_checks.port must not be equal.
    See https://docs.gitlab.com/charts/charts/gitlab/sidekiq/index.html#configuration

However, it should work when metrics are disabled:

helm upgrade -i gitlab gitlab/gitlab \
  --set gitlab.sidekiq.metrics.enabled=false \
  --set gitlab.sidekiq.health_checks.port=3807 \
  --set certmanager-issuer.email=me@example.com

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)

Edited by Matthias Käppler

Merge request reports