core:services:kraken: Fix container log stream timeout#3766
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntegrates a configurable-timeout Docker client context to prevent container log stream timeouts and uses it specifically for log retrieval, while keeping existing Docker usage unchanged elsewhere. Sequence diagram for container log retrieval with non expiring timeoutsequenceDiagram
actor Service
participant ContainerModel
participant DockerCtx
participant Docker
participant aiohttp_ClientSession
participant aiohttp_ClientTimeout
Service->>ContainerModel: get_container_log_by_name(container_name)
activate ContainerModel
ContainerModel->>DockerCtx: __init__(timeout=0)
activate DockerCtx
DockerCtx->>Docker: Docker(session=True)
activate Docker
Docker-->>DockerCtx: docker_instance
DockerCtx->>aiohttp_ClientTimeout: aiohttp_ClientTimeout(total=None, sock_read=None)
activate aiohttp_ClientTimeout
aiohttp_ClientTimeout-->>DockerCtx: timeout_instance
DockerCtx->>aiohttp_ClientSession: aiohttp_ClientSession(connector=docker_instance.connector, timeout=timeout_instance)
activate aiohttp_ClientSession
aiohttp_ClientSession-->>DockerCtx: session_instance
DockerCtx->>Docker: set session = session_instance
Docker-->>DockerCtx: session_configured
DockerCtx-->>ContainerModel: docker_instance as context
deactivate DockerCtx
ContainerModel->>Docker: get_container_logs(container_name)
Docker-->>ContainerModel: Async log stream (no total or sock_read timeout)
ContainerModel-->>Service: yield log lines
deactivate ContainerModel
Service-->>DockerCtx: exit async context
DockerCtx->>Docker: close()
Docker-->>DockerCtx: closed
DockerCtx-->>Service: context closed
Class diagram for updated DockerCtx and container log retrievalclassDiagram
class DockerCtx {
- Docker _client
+ DockerCtx(timeout Optional_int)
+ __aenter__() Docker
+ __aexit__(exc_type type, exc value, traceback object) None
}
class Docker {
+ connector aiohttp_BaseConnector
+ session aiohttp_ClientSession
+ Docker(session bool)
}
class aiohttp_ClientSession {
+ connector aiohttp_BaseConnector
+ timeout aiohttp_ClientTimeout
+ aiohttp_ClientSession(connector aiohttp_BaseConnector, timeout aiohttp_ClientTimeout)
}
class aiohttp_ClientTimeout {
+ total Optional_int
+ sock_read Optional_int
+ aiohttp_ClientTimeout(total Optional_int, sock_read Optional_int)
}
class ContainerModel {
+ get_container_log_by_name(container_name str) AsyncGenerator_str_None
+ get_raw_container_by_name(client Docker, container_name str) DockerContainer
}
DockerCtx --> Docker : creates
Docker --> aiohttp_ClientSession : uses
aiohttp_ClientSession --> aiohttp_ClientTimeout : uses
ContainerModel ..> DockerCtx : uses_for_log_stream
ContainerModel ..> Docker : uses_via_DockerCtx
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
b4181ca to
1f8a680
Compare
* Make sure kraken does not keeps reattaching to stream logs due to internal aiohttp 5 minutes timeout
|
Tried just the |
1f8a680 to
13c2ee0
Compare
|
@joaomariolago I believe we can link that PR to #3763, right? |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding
timeout=0as a magic value for "no timeout" inDockerCtx(e.g., use a named constant or a dedicated sentinel) and document that behavior in the class docstring so callers understand the semantics. - The assignment
self._client.session = self._client.session = aiohttp.ClientSession(...)inDockerCtx.__init__looks accidental; simplify this to a single assignment to make the intent clear.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding `timeout=0` as a magic value for "no timeout" in `DockerCtx` (e.g., use a named constant or a dedicated sentinel) and document that behavior in the class docstring so callers understand the semantics.
- The assignment `self._client.session = self._client.session = aiohttp.ClientSession(...)` in `DockerCtx.__init__` looks accidental; simplify this to a single assignment to make the intent clear.
## Individual Comments
### Comment 1
<location> `core/services/kraken/harbor/contexts.py:16-20` </location>
<code_context>
+ if timeout is None:
+ self._client: Docker = Docker()
+ else:
+ # aiodocker will not create a session if is different from None
+ self._client: Docker = Docker(session=True) # type: ignore
+ # We insert a new session with desired timeout
+ self._client.session = self._client.session = aiohttp.ClientSession(
+ connector=self._client.connector,
+ timeout=aiohttp.ClientTimeout(
+ total=None if timeout == 0 else timeout,
</code_context>
<issue_to_address>
**issue (bug_risk):** Closing or reusing the initially created session should be explicit to avoid potential resource leaks.
Using `Docker(session=True)` creates an internal `ClientSession` that is then replaced with a new one, leaving the original potentially unclosed. To avoid leaking resources, either construct the desired `ClientSession` first and pass it into `Docker`, or explicitly close the initial session before overwriting `self._client.session`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # aiodocker will not create a session if is different from None | ||
| self._client: Docker = Docker(session=True) # type: ignore | ||
| # We insert a new session with desired timeout | ||
| self._client.session = self._client.session = aiohttp.ClientSession( | ||
| connector=self._client.connector, |
There was a problem hiding this comment.
issue (bug_risk): Closing or reusing the initially created session should be explicit to avoid potential resource leaks.
Using Docker(session=True) creates an internal ClientSession that is then replaced with a new one, leaving the original potentially unclosed. To avoid leaking resources, either construct the desired ClientSession first and pass it into Docker, or explicitly close the initial session before overwriting self._client.session.
Closes #3763
Summary by Sourcery
Reduce the frequency of extension log synchronization by integrating it into the existing cleaner task loop instead of running a separate high-frequency background task.
Enhancements:
Summary by Sourcery
Allow configuring Docker client timeouts for Kraken harbor operations and use a non-timing-out client for container log streaming to prevent premature log stream termination.
Bug Fixes:
Enhancements: