fix(kubernetes): terminate idle computing units - #6046
Conversation
|
👋 Thanks for opening this pull request, @yrenat! It looks like the pull request description doesn't quite follow our template yet:
Filling out the template helps reviewers understand and triage your contribution faster. Please edit the description to complete it. This message will disappear automatically once the template is followed. You can find the template prompts by editing the description, or see CONTRIBUTING.md for the full contribution flow. |
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6046 +/- ##
============================================
- Coverage 92.77% 92.73% -0.05%
- Complexity 4900 4941 +41
============================================
Files 1236 1238 +2
Lines 52165 52386 +221
Branches 6411 6441 +30
============================================
+ Hits 48398 48581 +183
- Misses 2191 2207 +16
- Partials 1576 1598 +22
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 381 | 0.233 | 26,553/30,116/30,116 us | 🔴 +17.7% / 🔴 +108.8% |
| 🟢 | bs=100 sw=10 sl=64 | 803 | 0.49 | 121,870/144,980/144,980 us | 🟢 -5.4% / 🔴 +33.9% |
| ⚪ | bs=1000 sw=10 sl=64 | 921 | 0.562 | 1,080,533/1,164,521/1,164,521 us | ⚪ within ±5% / 🔴 +12.4% |
Baseline details
Latest main 6defa2c from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 381 tuples/sec | 422 tuples/sec | 775.4 tuples/sec | -9.7% | -50.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.233 MB/s | 0.258 MB/s | 0.473 MB/s | -9.7% | -50.8% |
| bs=10 sw=10 sl=64 | p50 | 26,553 us | 22,569 us | 12,716 us | +17.7% | +108.8% |
| bs=10 sw=10 sl=64 | p95 | 30,116 us | 33,977 us | 15,572 us | -11.4% | +93.4% |
| bs=10 sw=10 sl=64 | p99 | 30,116 us | 33,977 us | 19,527 us | -11.4% | +54.2% |
| bs=100 sw=10 sl=64 | throughput | 803 tuples/sec | 802 tuples/sec | 992.44 tuples/sec | +0.1% | -19.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.49 MB/s | 0.49 MB/s | 0.606 MB/s | 0.0% | -19.1% |
| bs=100 sw=10 sl=64 | p50 | 121,870 us | 120,945 us | 102,140 us | +0.8% | +19.3% |
| bs=100 sw=10 sl=64 | p95 | 144,980 us | 153,259 us | 108,282 us | -5.4% | +33.9% |
| bs=100 sw=10 sl=64 | p99 | 144,980 us | 153,259 us | 118,936 us | -5.4% | +21.9% |
| bs=1000 sw=10 sl=64 | throughput | 921 tuples/sec | 911 tuples/sec | 1,023 tuples/sec | +1.1% | -10.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.562 MB/s | 0.556 MB/s | 0.624 MB/s | +1.1% | -10.0% |
| bs=1000 sw=10 sl=64 | p50 | 1,080,533 us | 1,098,331 us | 998,263 us | -1.6% | +8.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,164,521 us | 1,161,548 us | 1,036,365 us | +0.3% | +12.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,164,521 us | 1,161,548 us | 1,064,941 us | +0.3% | +9.4% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,524.42,200,128000,381,0.233,26552.98,30116.03,30116.03
1,100,10,64,20,2489.78,2000,1280000,803,0.490,121870.00,144979.83,144979.83
2,1000,10,64,20,21709.33,20000,12800000,921,0.562,1080533.47,1164521.47,1164521.47
|
/request-review @aicam |
aicam
left a comment
aicam
left a comment
There was a problem hiding this comment.
Please add a new column to workflow_computing_unit which is "termination_reason" and we should have two types of termination (as of this PR):
- USER_REQUESTED
- GARBAGE_COLLECTED
aicam
left a comment
aicam
left a comment
There was a problem hiding this comment.
Please add a new column to workflow_computing_unit which is "termination_reason" and we should have two types of termination (as of this PR):
- USER_REQUESTED
- GARBAGE_COLLECTED
136f725 to
a11601f
Compare
|
this video shows the new effect: texera-PR6046.mp4 |
|
@yrenat please also have more tests. the coverage is too low. |
github-actions
Bot
added
the
dependencies
done. The coverage is now 100% |
github-actions
Bot
added
frontend
65c2db2 to
e551284
Compare
github-actions
Bot
added
dependencies
534bdad to
522a05d
Compare
github-actions
Bot
removed
the
dependencies
aicam
left a comment
aicam
left a comment
There was a problem hiding this comment.
LGTM, please take care of comments
|
One more thing that I think worthwhile to mention: In this PR branch, an idle Kubernetes CU by default becomes eligible after 24 hours idle, then it is terminated on the next scheduled cleanup sweep, which runs hourly by default. In this PR branch, it is configured here: It can be overridden by env var The cleanup job runs every 60 minutes by default: |
kunwp1
left a comment
kunwp1
left a comment
There was a problem hiding this comment.
Left some comments. Can you rebase on main, and update the name of the sql file, and add an entry to changelog.xml file?
45e178a to
1b56e3d
Compare
Done as suggested |
kunwp1
left a comment
kunwp1
left a comment
There was a problem hiding this comment.
I left some more comments. I want to be cautious on merging this PR so I want to have 1-2 more rounds of review.
1b56e3d to
2b226ec
Compare
kunwp1
left a comment
kunwp1
left a comment
There was a problem hiding this comment.
LGTM! Can you addess the comments?
|
For this comment @kunwp1 (#6046 (comment)): I created a following up issue #8618 |
Ma77Ball
left a comment
Ma77Ball
left a comment
There was a problem hiding this comment.
Overall LGTM! There doesn't appear to be any blocking issues. I left one inline comment below.
Adds backend-side cleanup for idle Kubernetes computing units, following the conclusion of discussion apache#6264. A scheduled task in the computing unit managing service periodically scans active Kubernetes computing units and terminates those idle longer than a configurable timeout: - new `computing-unit-idle-timeout-minutes` and `computing-unit-idle-check-interval-minutes` settings, both overridable by environment variable - one grouped query per sweep answers "is any execution still active", "when did one last report progress" and "when did one last start" for every unit at once - pod deletion happens outside the scan transaction and each unit's DB update runs in its own transaction, so one failure does not undo the batch - a non-positive check interval is logged and skipped rather than aborting service startup - `termination_reason` records whether a unit was terminated by the user or garbage collected `workflow_executions.cuid` gets its own index: Postgres indexes only the referenced side of a foreign key, so every per-computing-unit lookup on that table was a sequential scan. Closes apache#5362 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2b226ec to
be910af
Compare
7 participants
What changes were proposed in this PR?
Following discussion #6264, this PR adds backend-side cleanup for idle Kubernetes computing units.
The main change is a scheduled cleanup task in the computing unit managing service that periodically scans active Kubernetes computing units and terminates units that have been inactive longer than a configurable timeout.
The implementation includes the following changes:
ComputingUnitManagingServicethat runs the idle cleanup logic at a fixed interval.ComputingUnitManagingResource:The timeout and check interval are configurable through environment variables, so the behavior can be tuned for different deployment or testing needs without modifying the code.
Any related issues, documentation, discussions?
Fixes #5362
How was this PR tested?
Tested locally on the Kubernetes deployment flow.
fix-idle-CU-demo.mp4
Was this PR authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex GPT-5