Skip to content

CI: Fix terminating a process in a docker#88567

Merged
maxknv merged 7 commits intoClickHouse:masterfrom
leshikus:fix-timeouts-1
Oct 17, 2025
Merged

CI: Fix terminating a process in a docker#88567
maxknv merged 7 commits intoClickHouse:masterfrom
leshikus:fix-timeouts-1

Conversation

@leshikus
Copy link
Copy Markdown
Contributor

Changelog category (leave one):

@leshikus leshikus marked this pull request as draft October 15, 2025 11:26
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 15, 2025

Workflow [PR], commit [3fd9671]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 15, 2025
@leshikus
Copy link
Copy Markdown
Contributor Author

I don't like calling docker rm praktika from inside the abastract utility class but cannot think of anything better at the moment

@leshikus leshikus marked this pull request as ready for review October 15, 2025 11:32
@leshikus leshikus requested review from Copilot and maxknv October 15, 2025 14:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the issue of terminating processes running inside Docker containers when a timeout is exceeded. The key improvement is adding a customizable shell cleanup command that runs before attempting to terminate the process, allowing proper container cleanup.

Key Changes:

  • Added timeout_shell_cleanup parameter to enable custom cleanup logic during timeout handling
  • Modified timeout handling to execute cleanup command before attempting SIGTERM/SIGKILL
  • Set default cleanup command to remove Docker containers when jobs run in Docker

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
ci/praktika/utils.py Added timeout_shell_cleanup parameter to TeePopen class and modified _check_timeout method to execute cleanup command before termination signals
ci/praktika/runner.py Passed timeout_shell_cleanup parameter from job configuration to TeePopen
ci/praktika/job.py Added timeout_shell_cleanup field to job configuration and set default Docker container removal command in __post_init__

@leshikus
Copy link
Copy Markdown
Contributor Author

Here is a small note why I didn't just pass the docker container name to TeePopen - this class knows nothing about dockers, it is more like a shell utility.

@leshikus leshikus changed the title Fix terminating a process in a docker CI: Fix terminating a process in a docker Oct 16, 2025
@maxknv maxknv self-assigned this Oct 16, 2025
@leshikus leshikus requested a review from maxknv October 17, 2025 12:56
@maxknv maxknv added this pull request to the merge queue Oct 17, 2025
Merged via the queue into ClickHouse:master with commit 3e1bf49 Oct 17, 2025
123 checks passed
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 17, 2025
@leshikus leshikus deleted the fix-timeouts-1 branch March 9, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: Job timeouts failed to terminate jobs running in docker

4 participants