Remove supervisor docker binary requirement #2062
Conversation
🦋 Changeset detectedLatest commit: 7b83326 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update standardizes logging across multiple components by replacing direct console logging with a structured logger, introduces a no-op resource monitor for disabled resource monitoring, updates environment variable naming and schema for resource monitoring, and refactors Docker workload management to use the Dockerode API instead of CLI commands. It also adds a verbose log level and pretty-printing support to the structured logger, along with minor documentation and configuration adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant Supervisor as ManagedSupervisor
participant ResourceMonitor
participant NoopResourceMonitor
participant DockerWorkloadManager
participant DockerAPI as Dockerode
Env->>Supervisor: Provide environment variables
Supervisor->>ResourceMonitor: Initialize if RESOURCE_MONITOR_ENABLED
alt Resource monitoring enabled
Supervisor->>ResourceMonitor: Use Kubernetes or Docker monitor
else Resource monitoring disabled
Supervisor->>NoopResourceMonitor: Initialize NoopResourceMonitor
end
Supervisor->>DockerWorkloadManager: Create workload
DockerWorkloadManager->>DockerAPI: Create and start container via API
sequenceDiagram
participant Component as Any Component
participant Logger as SimpleStructuredLogger
participant Console
Component->>Logger: Log message (debug, error, verbose, etc.)
alt PRETTY_LOGS enabled
Logger->>Console: Output pretty-printed JSON log
else
Logger->>Console: Output compact JSON log
end
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/supervisor/.env.example (1)
15-18:⚠️ Potential issueAdd missing
RESOURCE_MONITOR_ENABLEDenvironment variable.The PR summary indicates that resource monitoring is now disabled by default and can be enabled via
RESOURCE_MONITOR_ENABLED. This example file should include that variable so users know how to opt in.Apply this diff to append the new setting:
# Optional settings DEBUG=1 ENFORCE_MACHINE_PRESETS=1 TRIGGER_DEQUEUE_INTERVAL_MS=1000 +# Enable resource monitor (disabled by default). Set to 1 or true to enable. +RESOURCE_MONITOR_ENABLED=
🧹 Nitpick comments (3)
apps/supervisor/src/workloadServer/index.ts (1)
397-403: Incorrect log message in dequeue handlerThe error message says "Failed to get latest snapshot" but this is in the dequeue handler. The message should reflect the actual operation being performed.
- this.logger.error("Failed to get latest snapshot", { + this.logger.error("Failed to dequeue from version", { deploymentId: params.deploymentId, error: dequeueResponse.error, });apps/supervisor/src/workloadManager/docker.ts (2)
16-17: Consider Docker connection options.The Docker client is initialized without explicit connection options, which will use default socket paths. This works for standard Docker installations, but consider making the connection configurable for environments with non-standard Docker daemon configurations.
- this.docker = new Docker(); + this.docker = new Docker(env.DOCKER_CONNECTION_OPTIONS || undefined);This would require adding a new optional environment variable to allow custom Docker connections.
82-91: Well-structured container creation options.The container creation options are comprehensive and properly typed. Consider adding a comment about why stdout/stderr and stdin attachments are set to false.
AttachStdout: false, AttachStderr: false, AttachStdin: false, + // Not attaching streams as we're collecting logs through the supervisor API
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/lazy-panthers-shop.md(1 hunks)apps/supervisor/.env.example(1 hunks)apps/supervisor/package.json(0 hunks)apps/supervisor/src/clients/kubernetes.ts(2 hunks)apps/supervisor/src/env.ts(1 hunks)apps/supervisor/src/envUtil.ts(2 hunks)apps/supervisor/src/index.ts(16 hunks)apps/supervisor/src/resourceMonitor.ts(5 hunks)apps/supervisor/src/workloadManager/docker.ts(1 hunks)apps/supervisor/src/workloadServer/index.ts(20 hunks)packages/core/src/v3/runEngineWorker/supervisor/http.ts(3 hunks)packages/core/src/v3/runEngineWorker/supervisor/queueConsumer.ts(5 hunks)packages/core/src/v3/runEngineWorker/supervisor/session.ts(7 hunks)packages/core/src/v3/schemas/common.ts(1 hunks)packages/core/src/v3/serverOnly/httpServer.ts(2 hunks)packages/core/src/v3/utils/structuredLogger.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/supervisor/package.json
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/core/src/v3/serverOnly/httpServer.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(20-93)
apps/supervisor/src/envUtil.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(20-93)
apps/supervisor/src/clients/kubernetes.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(20-93)
packages/core/src/v3/runEngineWorker/supervisor/http.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (2)
SimpleStructuredLogger(20-93)error(41-45)
apps/supervisor/src/env.ts (1)
apps/supervisor/src/envUtil.ts (1)
BoolEnv(6-12)
apps/supervisor/src/workloadManager/docker.ts (4)
apps/supervisor/src/workloadManager/types.ts (3)
WorkloadManager(16-18)WorkloadManagerOptions(3-14)WorkloadManagerCreateOptions(20-35)packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(20-93)apps/supervisor/src/util.ts (2)
getRunnerId(8-16)getDockerHostDomain(1-6)apps/supervisor/src/env.ts (1)
env(82-82)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (41)
apps/supervisor/.env.example (1)
10-10: Approve updated OTel endpoint comment.The comment now correctly clarifies that in production
OTEL_EXPORTER_OTLP_ENDPOINTcan point to either the webapp or an OpenTelemetry collector, improving clarity for users.packages/core/src/v3/schemas/common.ts (1)
119-121: Good documentation enhancement!Adding unit specifications to the schema fields improves clarity for developers working with these resources. This documentation makes it explicit that CPU is measured in vCPU units and memory in GB.
packages/core/src/v3/utils/structuredLogger.ts (2)
21-21: Nice addition of configurable log formatting.Adding the
prettyPrintflag based on environment variables gives developers flexibility to switch between compact and readable log formats depending on their needs. This is particularly useful during development and debugging.
87-91: Good implementation of conditional JSON formatting.The improved JSON formatting logic properly implements the
prettyPrintfeature. Using indentation (null, 2) for pretty printing while maintaining compact format by default is a good balance between readability and efficiency.packages/core/src/v3/serverOnly/httpServer.ts (2)
8-8: Better logger name that accurately reflects the component.Changing from "worker-http" to "http-server" provides a clearer indication of the logger's context, improving log readability and searchability.
127-127: Appropriate log level adjustment.Changing the request logging from
logtodebuglevel is appropriate for this type of verbose information. This change reduces noise in standard logs while still making the data available when detailed debugging is needed..changeset/lazy-panthers-shop.md (1)
1-6: Proper versioning documentation.The changeset correctly documents the structured logging improvements as a patch update, following good semantic versioning practices.
apps/supervisor/src/envUtil.ts (2)
2-4: Good adoption of structured logging!Nice implementation of the structured logger for the environment utilities module. Using a specialized logger with a descriptive name ("env-util") will make log collection and filtering more effective.
39-39: Proper replacement of console.warn with structured loggingReplacing direct console usage with the structured logger improves consistency in logging across the codebase and provides better context when viewing logs.
apps/supervisor/src/clients/kubernetes.ts (2)
6-6: Good structured logging implementationAdding the structured logger for the Kubernetes client component provides better logging organization. The descriptive logger name "kubernetes-client" will make log filtering more effective.
Also applies to: 10-11
37-37: Appropriate debug level loggingGood choice using the
debuglevel for this message, as it clearly represents diagnostic information rather than an error condition. This will maintain proper log categorization.packages/core/src/v3/runEngineWorker/supervisor/http.ts (2)
27-27: Well-implemented private loggerGood practice initializing the structured logger as a private readonly field with a descriptive name. This ensures consistent logging throughout the class and prevents accidental modification of the logger instance.
Also applies to: 37-38
223-223: Improved error logging in debug log sendingProperly updated the error logging with structured logging, maintaining the same error context in both error cases. This provides more consistent error reporting and will make debugging failures in the debug log sending functionality easier.
Also applies to: 226-226
packages/core/src/v3/runEngineWorker/supervisor/queueConsumer.ts (3)
1-1: Good structured logger implementationAdded structured logging with a descriptive "queue-consumer" logger name, which helps with log organization and filtering.
Also applies to: 23-24
59-59: Cleanup of verbose debug statementsGood decision to comment out extremely verbose debug statements while retaining the error logging. As noted in the comment on line 58, these were "incredibly verbose" and likely only useful for deep debugging sessions.
Also applies to: 67-67, 76-76
72-72: Consistent error logging patternWell-implemented replacement of console logging with structured logging across all error cases in the dequeue process. The error contexts are maintained properly, preserving the original debugging information while standardizing the format.
Also applies to: 84-84, 89-89, 105-105, 114-114, 118-118
apps/supervisor/src/env.ts (1)
52-54: Well-structured environment variable updates for resource monitoringThe changes appropriately introduce a new boolean flag
RESOURCE_MONITOR_ENABLEDwith a default value offalse, which aligns with the PR objective of making resource monitoring optional. The naming of the CPU and memory override variables has been standardized with theRESOURCE_MONITOR_prefix, improving clarity and organization in the environment schema.These changes provide a clear configuration path for users who want to enable resource monitoring through the
RESOURCE_MONITOR_ENABLEDenvironment variable.apps/supervisor/src/workloadServer/index.ts (4)
66-67: Standardized logging with SimpleStructuredLoggerGood introduction of a structured logger instance with a descriptive context name "workload-server".
156-162: Consistent error logging patternThe logging implementation correctly includes both the parameters and error information, which will help with debugging failures.
424-447: Improved socket header validation with better error loggingThe error handling is now more informative with structured logging that includes the specific header name that's missing.
463-467: Good use of child logger for socket contextUsing a child logger that inherits the parent context and adds socket-specific metadata is a good practice. This will help correlate log messages related to the same socket connection.
packages/core/src/v3/runEngineWorker/supervisor/session.ts (3)
29-30: Standardized logging with properly named contextGood introduction of a structured logger with the context "supervisor-session", which clearly identifies the source of log messages.
172-173: Simplified error message formatThe error message format was simplified by removing the "[HTTP]" prefix. This is consistent with the move to structured logging where the context is already provided by the logger instance.
144-149:✅ Verification successful
Removed error handling around sendDebugLog
The try-catch block around the
sendDebugLogcall has been removed. Make sure error handling is implemented within thesendDebugLogmethod itself to prevent unhandled exceptions.
🏁 Script executed:
#!/bin/bash # Check if sendDebugLog has internal error handling rg -A 5 -B 5 "sendDebugLog" "packages/core/src/v3/runEngineWorker/supervisor/http.ts"Length of output: 370
🏁 Script executed:
#!/bin/bash # Display the full sendDebugLog function implementation to verify error handling rg -A 50 -B 0 "async sendDebugLog" "packages/core/src/v3/runEngineWorker/supervisor/http.ts"Length of output: 1503
Internal error handling already present in sendDebugLog; no external try-catch needed
The
sendDebugLogmethod inpackages/core/src/v3/runEngineWorker/supervisor/http.tswraps its own call in atry…catchand logs both non-success responses and thrown errors. No additional error handling is required at the call site.• File: packages/core/src/v3/runEngineWorker/supervisor/http.ts
–async sendDebugLog(…) { try { … } catch (error) { this.logger.error(…) } }No changes necessary.
apps/supervisor/src/index.ts (4)
73-77: Properly implemented optional resource monitoringThe resource monitor initialization now correctly uses a
NoopResourceMonitorwhen resource monitoring is disabled, while maintaining the existing functionality when enabled. This aligns with the PR objective of making resource monitoring optional.
79-81: Improved code organization for workload manager initializationThe workload manager initialization has been moved outside the Kubernetes conditional block, which reduces code duplication and improves readability.
128-130: Efficient early return for disabled resource monitoringAdded an early return in the
preDequeuefunction when resource monitoring is disabled, which avoids unnecessary processing. This is a good optimization.
37-37: Updated logger name for clarityThe logger context name has been updated from "managed-worker" to "managed-supervisor", which better reflects the actual class name and function.
apps/supervisor/src/workloadManager/docker.ts (7)
9-9: Good addition of Dockerode dependency.The transition from Docker CLI commands to the Dockerode API eliminates the dependency on the Docker binary being installed on the host system, aligning with the PR objective.
12-13: Improved logger naming and added Docker client.The logger name change from "docker-workload-provider" to "docker-workload-manager" better reflects the class name, improving consistency. The addition of the private Docker client as a class member is appropriate for the Dockerode API approach.
31-43: Good implementation of environment variables.The approach of collecting environment variables in an array is clean and maintainable. All essential environment variables are properly set for the container.
69-72: Well-structured host configuration.The Docker host configuration is properly typed and includes essential settings for network mode and container cleanup. Great use of TypeScript types from the Dockerode library.
75-80: Correct resource limits implementation.The implementation properly sets both environment variables and Docker resource constraints when machine presets are enforced. The CPU conversion to nanocpus (cores * 1e9) and memory conversion to bytes are accurate.
94-100: Good error handling and logging.The implementation properly creates and starts the container using the Dockerode API with appropriate error handling and debug logging.
102-102: Comprehensive error logging.Including the container creation options in the error log is valuable for troubleshooting container startup issues.
apps/supervisor/src/resourceMonitor.ts (6)
73-88: Good environment variable name updates.The environment variable names have been updated with a more specific prefix (
RESOURCE_MONITOR_) which improves clarity and organization. The implementation correctly handles the case when overrides are not set.
105-108: Well-defined SystemInfo type.The new
SystemInfotype properly defines the Docker system info structure with optional properties, which handles potential undefined values gracefully.
124-124: Appropriate use of the new SystemInfo type.The Docker info call is now properly typed with the custom SystemInfo type, improving type safety.
218-231: Well-implemented NoopResourceMonitor.The
NoopResourceMonitorclass is a clean implementation that fulfills the PR objective of disabling resource monitoring by default. SettingcpuAvailableandmemoryAvailabletoInfinityis a smart approach that ensures no resource constraints are applied when monitoring is disabled.
270-278: Simple and effective NoopResourceParser.The
NoopResourceParserimplementation is appropriately minimal, passing through values unchanged which is perfect for a no-op implementation.
218-278:✅ Verification successful
Verify NoopResourceMonitor integration.
The PR objectives mention that resource monitoring is disabled by default. Ensure that this
NoopResourceMonitorclass is properly integrated in the supervisor's initialization code.
🏁 Script executed:
#!/bin/bash # Check how the NoopResourceMonitor is used in the codebase rg -A 5 -B 5 "new NoopResourceMonitor" # Look for the RESOURCE_MONITOR_ENABLED environment variable rg "RESOURCE_MONITOR_ENABLED" --include "*.ts"Length of output: 1274
Let’s confirm how
RESOURCE_MONITOR_ENABLEDis defined and what its default value is:
🏁 Script executed:
#!/bin/bash # 1. List all occurrences of RESOURCE_MONITOR_ENABLED in supervisor source rg -n "RESOURCE_MONITOR_ENABLED" apps/supervisor/src # 2. Locate the environment config file fd env.ts apps/supervisor/src # 3. Show the definition and defaults in env.ts sed -n '1,200p' apps/supervisor/src/env.tsLength of output: 3954
NoopResourceMonitor is correctly integrated
The supervisor’s startup in
apps/supervisor/src/index.tsusesenv.RESOURCE_MONITOR_ENABLED(defaultfalse) to select:
new KubernetesResourceMonitor(...)ornew DockerResourceMonitor(...)when enablednew NoopResourceMonitor()when disabledNo further action is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
96-100: Consider extracting environment variable parsing logicThe environment variable parsing logic
["1", "true"].includes(...)is duplicated in multiple places. Consider extracting this to a utility function for better maintainability.+ private isEnabled(envVar: string | undefined): boolean { + return ["1", "true"].includes(envVar ?? ""); + } + private prettyPrint = ["1", "true"].includes(process.env.PRETTY_LOGS ?? ""); constructor( private name: string, - private level: LogLevel = ["1", "true"].includes(process.env.VERBOSE ?? "") + private level: LogLevel = this.isEnabled(process.env.VERBOSE) ? LogLevel.verbose - : ["1", "true"].includes(process.env.DEBUG ?? "") + : this.isEnabled(process.env.DEBUG) ? LogLevel.debug : LogLevel.info, private fields?: Record<string, unknown> ) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.changeset/rare-beds-accept.md(1 hunks)apps/supervisor/src/env.ts(2 hunks)apps/supervisor/src/index.ts(16 hunks)apps/supervisor/src/workloadManager/docker.ts(1 hunks)packages/core/src/v3/runEngineWorker/supervisor/queueConsumer.ts(5 hunks)packages/core/src/v3/runEngineWorker/supervisor/session.ts(7 hunks)packages/core/src/v3/utils/structuredLogger.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/rare-beds-accept.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/v3/runEngineWorker/supervisor/queueConsumer.ts
- packages/core/src/v3/runEngineWorker/supervisor/session.ts
- apps/supervisor/src/index.ts
- apps/supervisor/src/env.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
packages/core/src/v3/utils/structuredLogger.ts (4)
18-18: Good addition of a verbose log levelAdding a "verbose" level below "debug" in the LogLevel enum is a good enhancement for even more granular logging when needed.
22-22: Nice addition of pretty-printing supportThe
prettyPrintflag based on thePRETTY_LOGSenvironment variable is a useful feature for development and debugging. This will make logs much more readable when needed.
26-29: Improved log level determination with VERBOSE supportThe enhanced constructor logic properly prioritizes environment variables (VERBOSE → DEBUG → default INFO) for determining the log level. This aligns well with the new verbose functionality.
68-72: Good implementation of the verbose methodThe new
verbose()method follows the same pattern as other logging methods and correctly usesconsole.debugas the underlying function.apps/supervisor/src/workloadManager/docker.ts (9)
9-10: Good addition of required dependencies for Docker API usage.The import of
dockerodeandtryCatchis necessary for the refactoring from CLI commands to API calls, which aligns with the PR objective of removing the Docker binary requirement.
13-15: Appropriate updates to logger name and Docker client initialization.The logger name change from "docker-workload-provider" to "docker-workload-manager" better reflects the class name. The addition of a private Docker client instance using Dockerode is a good implementation choice.
16-28: Good approach for network configuration from environment variables.Storing runner networks as a private array and loading from the environment variable is a clean implementation. This change aligns with the standardization mentioned in the PR objectives.
36-48: Well-structured environment variables collection.The approach of collecting environment variables in a string array is cleaner and more maintainable than the previous CLI argument approach.
74-84: Great implementation of multi-network support.The code now properly handles multiple Docker networks with clear comments explaining the approach. Using the first network as the primary NetworkMode and attaching additional networks later is an efficient implementation strategy.
85-91: Proper implementation of machine preset enforcement.Setting both environment variables and Docker resource limits (using NanoCpus and Memory) ensures the runner has access to the correct resource information and is properly constrained.
93-102: Well-structured container creation options.The Docker.ContainerCreateOptions object is well-organized with all necessary parameters for container creation, making the code more readable and maintainable.
104-128: Good container creation and error handling.The container creation, network attachment, and starting process is well-implemented with proper error handling and detailed logging. This will make debugging issues much easier.
130-174: Well-implemented network attachment method.The
attachContainerToNetworksmethod is well-designed with:
- Proper error handling using
tryCatch- Concurrent network attachment using
Promise.allSettled- Exact network name matching to prevent unexpected results
- Comprehensive logging for both success and failure cases
This implementation enhances the flexibility of the Docker workload manager.
Other stuff:
RESOURCE_MONITOR_ENABLEDVERBOSE=1Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores