Skip to content

fix(deployment): Use unique Celery node names per worker replica. #2269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
junhaoliao merged 3 commits into from
May 14, 2026
+5 −5
Merged

fix(deployment): Use unique Celery node names per worker replica. #2269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
424e626
fix(deployment): Use unique Celery node names per pod to prevent pidb…
May 11, 2026
a3b8f01
fix(deployment): Also update docker-compose worker node names to use %h
May 13, 2026
7c0c37e
Merge branch 'main' into fix/celery-worker-unique-node-name
junhaoliao May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/deployment/package-helm/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: "v2"
name: "clp"
version: "0.3.2-dev.4"
Copy link
Copy Markdown
Member

@junhaoliao junhaoliao May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the git branch's base seems to be out of date. please merge origin (y-scope)'s main branch into the PR source branch

version: "0.3.2-dev.5"
description: "A Helm chart for CLP's (Compressed Log Processor) package deployment"
type: "application"
appVersion: "0.12.1-dev"
Expand Down
2 changes: 1 addition & 1 deletion tools/deployment/package-helm/templates/compression-worker-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ spec:
"--concurrency", "{{ .Values.workerConcurrency }}",
"--loglevel", "WARNING",
"-Q", "compression",
"-n", "compression-worker"
"-n", "compression-worker@%h"
Copy link
Copy Markdown
Member

@junhaoliao junhaoliao May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibber9809 and i discussed offline - let's update https://github.com/y-scope/clp/blob/aeb077ca4/tools/deployment/package/docker-compose-all.yaml#L329 to match this command argument

though, practically, that fix won't be enough to ensure uniqueness in the docker compose services:

  1. we hardcode some service name at:

    clp/tools/deployment/package/docker-compose-all.yaml

    Line 298 in aeb077c

    hostname: "compression_worker"
    . then if we ever scale the compression worker to multiple replicas, each replica reads the same hostname which results in same worker names across all replicas
  2. we should avoid hardcoding the hostname then. that way, docker compose will assign numeric post-fixes to the hostnames (e.g. compression-worker-1), ensuring uniqueness of pidbox names when used by Celery, while the DNS can still resolve compression-worker to different IPs of the replicas.
  3. however, that why we hardcoded hostname for every docker-compose service was to ensure their hostnames match the "component names", defined in Python CLP utils, e.g.

    clp/components/clp-py-utils/clp_py_utils/clp_config.py

    Line 42 in 0144808

    COMPRESSION_WORKER_COMPONENT_NAME = "compression_worker"
    . those "component names" use _ in names, which differs from the docker-compose service names where - are used as separators. We should also calibrate the component names (as tracked by Standardise service names to use hyphens instead of underscores for RFC1123 compliance #1777 ; refactor(deployment): Standardizes service names to use hyphens instead of underscores for RFC1123 compliance (resolves #1777). #1785 has propsoed a fix) to match the service names.

since satisfying 2 + 3 will involve changing a lot of code, i think simply aligning the command in docker-compose-all.yaml (e.g. change "compression-worker" -> "compression-worker@h") is sufficient for the scope of this PR. we shall fix 2 + 3 seperately.

Copy link
Copy Markdown
Member

@junhaoliao junhaoliao May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that said, as pointed out by @gibber9809 , the above proposal will only ensure uniqueness across different compression-worker replicas on the same host where the docker-compose project runs. (which is still valuable as we can debug multi worker issues without a multi host cluster)

however, if the docker compose project is run across multiple hosts to form a cluster, hostnames can still collide. as proposed by #2274 , maybe we should formally remove mulithost support in the docker compose flow

]
volumes:
- name: {{ include "clp.volumeName" (dict
Expand Down
2 changes: 1 addition & 1 deletion tools/deployment/package-helm/templates/query-worker-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ spec:
"--concurrency", "{{ .Values.workerConcurrency }}",
"--loglevel", "WARNING",
"-Q", "query",
"-n", "query-worker"
"-n", "query-worker@%h"
Copy link
Copy Markdown
Member

@junhaoliao junhaoliao May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto - let's update https://github.com/y-scope/clp/blob/aeb077ca4/tools/deployment/package/docker-compose-all.yaml#L516 to match this command argument

]
volumes:
- name: {{ include "clp.volumeName" (dict
Expand Down
Loading