Add supervisor split service control #1774
Conversation
|
WalkthroughThis pull request refactors environment variable handling and associated configuration across several components. It introduces a new boolean preprocessing schema ( Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Variables
participant MS as ManagedSupervisor
participant WS as Workload Server
participant Log as Logger
MS->>Env: Read TRIGGER_WORKLOAD_API_ENABLED and API config
alt Workload API Enabled
MS->>Log: Log protocol, domain, and port
MS->>WS: Start workload server with new config
else Workload API Disabled
MS->>Log: Log warning "workload API disabled"
end
sequenceDiagram
participant Sess as SupervisorSession
participant RS as RunNotificationsSocket
participant QC as QueueConsumer
Sess->>Sess: Read runNotificationsEnabled and queueConsumerEnabled
alt runNotificationsEnabled
Sess->>RS: Create run notifications socket
RS-->>Sess: Socket initialized
else
Sess->>Log: Log notifications disabled
end
alt queueConsumerEnabled
Sess->>QC: Initialize queue consumer service
else
Sess->>Log: Log consumer disabled
end
Possibly related PRs
Poem
✨ 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:
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
🧹 Nitpick comments (4)
apps/supervisor/src/index.ts (1)
249-258: Conditional workload API server startupGood addition of a feature toggle with appropriate logging for both enabled and disabled states. This allows for more deployment flexibility.
Consider adding error handling for cases where workload API server fails to start when enabled:
if (env.TRIGGER_WORKLOAD_API_ENABLED) { this.logger.log("[ManagedWorker] Workload API enabled", { protocol: env.TRIGGER_WORKLOAD_API_PROTOCOL, domain: env.TRIGGER_WORKLOAD_API_DOMAIN, port: env.TRIGGER_WORKLOAD_API_PORT_INTERNAL, }); - await this.workloadServer.start(); + try { + await this.workloadServer.start(); + } catch (error) { + this.logger.error("[ManagedWorker] Failed to start Workload API server", { error }); + // Consider whether to throw or continue with degraded functionality + } } else { this.logger.warn("[ManagedWorker] Workload API disabled"); }packages/core/src/v3/runEngineWorker/supervisor/session.ts (1)
37-38: Default values for feature togglesSetting sensible defaults for the new feature toggles ensures backward compatibility.
Consider moving these defaults to the type definition for improved clarity:
type SupervisorSessionOptions = SupervisorClientCommonOptions & { - queueConsumerEnabled?: boolean; - runNotificationsEnabled?: boolean; + queueConsumerEnabled?: boolean; // defaults to true + runNotificationsEnabled?: boolean; // defaults to true heartbeatIntervalSeconds?: number; dequeueIntervalMs?: number; preDequeue?: PreDequeueFn; preSkip?: PreSkipFn; };apps/supervisor/src/workloadManager/docker.ts (2)
33-35: Environment variable restructuringBreaking down the previously monolithic
TRIGGER_WORKER_API_URLinto component parts enhances configurability and aligns with the structure changes elsewhere in the codebase.The fallback to
getDockerHostDomain()is appropriate, but consider adding error handling:+ // Ensure we have a valid domain for the supervisor API + const supervisorDomain = this.opts.workloadApiDomain ?? getDockerHostDomain(); + if (!supervisorDomain) { + this.logger.error("[DockerWorkloadProvider] Failed to determine supervisor domain"); + } `--env=TRIGGER_SUPERVISOR_API_PROTOCOL=${this.opts.workloadApiProtocol}`, `--env=TRIGGER_SUPERVISOR_API_PORT=${this.opts.workloadApiPort}`, - `--env=TRIGGER_SUPERVISOR_API_DOMAIN=${this.opts.workloadApiDomain ?? getDockerHostDomain()}`, + `--env=TRIGGER_SUPERVISOR_API_DOMAIN=${supervisorDomain}`,
56-60: Consider propagating errors in create methodThe current implementation logs errors but doesn't propagate them to callers, potentially masking issues.
try { const { stdout, stderr } = await x("docker", runArgs); this.logger.debug("[DockerWorkloadProvider] Create succeeded", { stdout, stderr }); } catch (error) { this.logger.error("[DockerWorkloadProvider] Create failed:", { opts, error }); + throw error; // Propagate the error to allow callers to handle it }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/supervisor/src/env.ts(1 hunks)apps/supervisor/src/index.ts(5 hunks)apps/supervisor/src/workloadManager/docker.ts(2 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(2 hunks)apps/supervisor/src/workloadManager/types.ts(1 hunks)packages/cli-v3/src/entryPoints/managed-run-controller.ts(4 hunks)packages/core/src/v3/runEngineWorker/supervisor/session.ts(7 hunks)
⏰ 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 (17)
apps/supervisor/src/workloadManager/kubernetes.ts (2)
27-31: Good addition of warning log for custom workload API domain.This warning message provides useful visibility when a custom domain is being used, helping with debugging and understanding system configuration.
81-100: Environment variable restructuring looks good.The change from a single API URL to separate environment variables for protocol, port, and domain provides better flexibility. The conditional logic for domain specification is well-implemented, falling back to the pod's host IP when no custom domain is provided.
apps/supervisor/src/workloadManager/types.ts (1)
4-6: Well-structured API configuration interface.Breaking down the workload API configuration into separate components (protocol, domain, port) is a good design choice that provides more flexibility than a single URL. The comment for
workloadApiDomainclearly explains the fallback behavior.apps/supervisor/src/env.ts (2)
5-11: Good implementation of boolean environment variable parsing.The
BoolEnvpreprocessor nicely handles string-to-boolean conversion for environment variables, supporting both "true" and "1" values. This improves type safety and reduces potential runtime errors.
22-34: Well-organized configuration with clear section comments.The workload API settings are well-structured with appropriate defaults and types. Breaking out the configuration into logical sections with comments makes the code more maintainable. The transformation for
TRIGGER_WORKLOAD_API_PROTOCOLensures valid values.packages/cli-v3/src/entryPoints/managed-run-controller.ts (3)
39-41: Environment variables properly aligned with other components.These new environment variables correctly match the refactored approach used in other files, ensuring consistency across the system.
89-89: Appropriate construction of worker API URL from components.The
workerApiUrlis now properly constructed from separate protocol, domain, and port components, which aligns with the configuration changes in other files.Also applies to: 253-253
755-755: Updated socket creation to use the new URL structure.The WebSocket URL creation now correctly uses the refactored
workerApiUrlproperty, maintaining consistent connectivity behavior.apps/supervisor/src/index.ts (4)
32-34: Enhanced configuration modularity introducedThe introduction of separate variables for workload API configuration components (protocol, domain, port) provides more granular control than using a single URL.
45-48: Configuration structure improvement for workload managersBreaking down the workload API URL into component parts (protocol, domain, port) enhances flexibility and maintainability.
Also applies to: 53-56
66-67: Feature toggles added for supervisor session capabilitiesThe new properties enable granular control over queue consumption and run notification features, allowing them to be independently enabled/disabled.
191-191: Port naming clarificationChanging from
TRIGGER_WORKLOAD_API_PORTtoTRIGGER_WORKLOAD_API_PORT_INTERNALbetter reflects the purpose of the port and distinguishes it from external-facing configurations.packages/core/src/v3/runEngineWorker/supervisor/session.ts (4)
14-15: Extended configurability for SupervisorSessionAdded optional properties enhance the configurability of the session by allowing specific features to be toggled.
25-28: Socket property refactoring for clarityRenaming
sockettorunNotificationsSocketclarifies the purpose of the socket instance, improving code readability.
127-159: Renamed method reflects specific purposeRenaming
createSockettocreateRunNotificationsSocketbetter describes the method's purpose, improving code clarity.
180-193: Feature toggle implementation for session servicesThe conditional initialization of services based on feature toggles provides deployment flexibility while maintaining appropriate logging.
apps/supervisor/src/workloadManager/docker.ts (1)
14-20: Added logging for custom workload API domain configurationGood addition of warning logs when custom domain is used, improving observability of non-default configurations.
Also has orchestrator-specific fixes for the workload api domain
Summary by CodeRabbit
New Features
Refactor