-
Notifications
You must be signed in to change notification settings - Fork 30
✨ Adds t_scheduler used for reliably running code in dynamic-scheduler ⚠️
#9036
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
691d3c4
06584b1
4619e87
2088067
2639497
dffeb94
86ae8c7
d3545ab
73e089f
3c81f8d
43ad06d
c7a26de
6b53262
60e4d4f
e268206
a4f2628
7af2d4b
188f6a8
9e512da
e96b927
058f579
e9b7446
d86e337
df1100d
0d01e45
a0e9823
4efb745
f0cb012
90e7b39
6424716
03fa767
f5658ed
3cf8b9c
8832aed
a733764
98a5288
7a3df9a
99791b9
8d0119d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| name: OPS Temporalio Maintenance Reminder | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| paths: | ||
| - "services/dynamic-scheduler/workflows_signatures.json" | ||
| types: [opened, synchronize] | ||
|
|
||
| jobs: | ||
| ops-temporalio-maintenance-comment: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| pull-requests: write | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Post OPS maintenance comment | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: > | ||
| ./ci/github/helpers/ops-temporal-maintenance-comment.bash | ||
| "${{ github.repository }}" | ||
| "${{ github.event.pull_request.number }}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| #!/bin/bash | ||
| # Posts a one-time PR comment when workflows_signatures.json changes, | ||
| # warning OPS that Temporalio workflows must be shut down before deploying. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it not be possible to handle automate this step completely? Either by always shutting down the temporalio thingy, or by having a health check which checks that the workflows are up to date. Having a separate CI job for posting a message to a PR seems very cumbersome and error prone to me. |
||
| # | ||
| # Usage: | ||
| # bash ci/github/helpers/ops-temporal-maintenance-comment.bash <repo> <pr_number> | ||
| # | ||
| # Environment: | ||
| # GH_TOKEN — GitHub token with pull-requests:write scope | ||
|
|
||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
| IFS=$'\n\t' | ||
|
|
||
| REPO=$1 | ||
| PR_NUMBER=$2 | ||
| MARKER="OPS-TEMPORALIO-MAINTENANCE-REQUIRED" | ||
| TARGET_FILE="services/dynamic-scheduler/workflows_signatures.json" | ||
|
|
||
| # Check if comment already exists | ||
| EXISTING=$(gh api \ | ||
| "repos/${REPO}/issues/${PR_NUMBER}/comments" \ | ||
| --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" \ | ||
| | head -1) | ||
|
|
||
| if [ -n "$EXISTING" ]; then | ||
| echo "Comment already exists (id=${EXISTING}), skipping." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Post the comment | ||
| gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" \ | ||
| --method POST \ | ||
| --field body="<!-- ${MARKER} --> | ||
| ## ⚠️ Temporalio Maintenance Required Before Deploy | ||
|
|
||
| This PR modifies \`${TARGET_FILE}\`, which means **workflow or activity implementations have changed**. | ||
|
|
||
| Before deploying, OPS **must** shut down all running Temporalio workflows to prevent stale executions. | ||
|
|
||
| **Steps:** | ||
| 1. Do not merge without notifying OPS | ||
| 2. OPS shuts down running workflows via \`POST /ops/temporalio-workflows:shutdown\` | ||
| 3. Deploy the new code | ||
| 4. Resolve/acknowledge this comment once confirmed ✅ | ||
|
|
||
| _Triggered automatically because \`${TARGET_FILE}\` was changed._" | ||
|
|
||
| echo "✅ Comment posted." | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| #!/bin/bash | ||
| # http://redsymbol.net/articles/unofficial-bash-strict-mode/ | ||
| set -o errexit # abort on nonzero exitstatus | ||
| set -o nounset # abort on unbound variable | ||
| set -o pipefail # don't hide errors within pipes | ||
| IFS=$'\n\t' | ||
|
|
||
| install() { | ||
| make devenv | ||
| # shellcheck source=/dev/null | ||
| source .venv/bin/activate | ||
| pushd services/dynamic-scheduler | ||
| make install-ci | ||
| popd | ||
| uv pip list | ||
| make info-images | ||
| } | ||
|
|
||
| test() { | ||
| # shellcheck source=/dev/null | ||
| source .venv/bin/activate | ||
| pushd services/dynamic-scheduler | ||
| make test-ci-integration | ||
| popd | ||
| } | ||
|
|
||
| clean_up() { | ||
| docker images | ||
| make down | ||
| } | ||
|
|
||
| # Check if the function exists (bash specific) | ||
| if declare -f "$1" >/dev/null; then | ||
| # call arguments verbatim | ||
| "$@" | ||
| else | ||
| # Show a helpful error | ||
| echo "'$1' is not a known function name" >&2 | ||
| exit 1 | ||
| fi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| from datetime import timedelta | ||
| from functools import cached_property | ||
| from typing import Annotated | ||
|
|
||
| from pydantic import Field | ||
|
|
||
| from .base import BaseCustomSettings | ||
| from .basic_types import PortInt | ||
|
|
||
|
|
||
| class TemporalioSettings(BaseCustomSettings): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. temporal.io is the website and the python SDK right? |
||
| TEMPORALIO_HOST: Annotated[ | ||
| str, | ||
| Field(description="Hostname of the Temporalio server gRPC endpoint"), | ||
| ] = "temporal" | ||
|
|
||
| TEMPORALIO_PORT: Annotated[ | ||
| PortInt, | ||
| Field(description="Port of the Temporalio server gRPC endpoint"), | ||
| ] = 7233 | ||
|
|
||
| TEMPORALIO_NAMESPACE: Annotated[ | ||
| str, | ||
| Field(description="Temporalio namespace to use for workflows"), | ||
| ] = "default" | ||
|
|
||
| TEMPORALIO_TASK_QUEUE: Annotated[ | ||
| str, | ||
| Field(description="Temporalio task queue name"), | ||
| ] = "dynamic-scheduler" | ||
|
|
||
| TEMPORALIO_WORKER_GRACEFUL_SHUTDOWN_TIMEOUT: Annotated[ | ||
| timedelta, | ||
| Field( | ||
| description=( | ||
| "Time the Temporalio worker waits for running activities to complete " | ||
| "before cancelling them during shutdown. " | ||
| "Must be less than docker-compose stop_grace_period for the service." | ||
| ), | ||
| ), | ||
| ] = timedelta(seconds=30) | ||
|
|
||
| @cached_property | ||
| def target_host(self) -> str: | ||
| return f"{self.TEMPORALIO_HOST}:{self.TEMPORALIO_PORT}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -358,6 +358,23 @@ services: | |
| - default | ||
| - interactive_services_subnet # for legacy dynamic services | ||
|
|
||
| temporal: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't you add a custom healthcheck to this service which queries the dynamic scheduler to get the workflow signature (or maybe simply a hash of it) and fails if it doesn't match what it expects. That way OPS would not need to be involved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I get it right, that the temporal service is where the different "sagas" are registered? |
||
| # Temporal server with auto schema setup — https://docs.temporal.io/self-hosted-guide | ||
| # Reuses the existing postgres service; auto-creates "temporal" and "temporal_visibility" databases. | ||
| image: temporalio/auto-setup:1.29.1 | ||
| init: true | ||
| hostname: "{{.Node.Hostname}}-{{.Task.Slot}}" | ||
| environment: | ||
| DB: postgres12 | ||
| DB_PORT: 5432 | ||
| POSTGRES_USER: ${POSTGRES_USER} | ||
| POSTGRES_PWD: ${POSTGRES_PASSWORD} | ||
| POSTGRES_SEEDS: postgres | ||
| TEMPORAL_ADDRESS: temporal:7233 | ||
| BIND_ON_IP: 0.0.0.0 | ||
| networks: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. healthcheck is missing here. please add one thanks |
||
| - default | ||
|
|
||
| ##################################### | ||
| ### Group simcore no dependencies ### | ||
| ##################################### | ||
|
|
@@ -876,6 +893,9 @@ services: | |
| dynamic-schdlr: | ||
| image: ${DOCKER_REGISTRY:-itisfoundation}/dynamic-scheduler:${DOCKER_IMAGE_TAG:-latest} | ||
| init: true | ||
| # Must be greater than TEMPORALIO_WORKER_GRACEFUL_SHUTDOWN_TIMEOUT (default 30s) | ||
| # to allow the Temporalio worker to finish running activities before Docker sends SIGKILL. | ||
| stop_grace_period: 45s | ||
| hostname: "{{.Node.Hostname}}-{{.Task.Slot}}" | ||
| networks: | ||
| - default | ||
|
|
@@ -904,6 +924,7 @@ services: | |
| DYNAMIC_SCHEDULER_STOP_SERVICE_TIMEOUT: ${DYNAMIC_SCHEDULER_STOP_SERVICE_TIMEOUT} | ||
| DYNAMIC_SCHEDULER_TRACING: ${DYNAMIC_SCHEDULER_TRACING} | ||
| DYNAMIC_SCHEDULER_UI_STORAGE_SECRET: ${DYNAMIC_SCHEDULER_UI_STORAGE_SECRET} | ||
| DYNAMIC_SCHEDULER_TEMPORALIO_SETTINGS: ${DYNAMIC_SCHEDULER_TEMPORALIO_SETTINGS} | ||
| DYNAMIC_SCHEDULER_USE_INTERNAL_SCHEDULER: ${DYNAMIC_SCHEDULER_USE_INTERNAL_SCHEDULER} | ||
| DYNAMIC_SIDECAR_API_SAVE_RESTORE_STATE_TIMEOUT: ${DYNAMIC_SIDECAR_API_SAVE_RESTORE_STATE_TIMEOUT} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to avoid having this job if possible (see also my other comment)