Misc v4 fixes and improved logging #1831
Conversation
|
WalkthroughThis pull request introduces several targeted changes across the codebase. In the supervisor app, multiple new environment variables are added to the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (3)
apps/supervisor/README.md (1)
46-49: Missing comma after "worker groups"There should be a comma after "worker groups" to properly separate the introductory clause from the main clause.
-When adding more worker groups you might also want to make them the default for a specific project. This will allow you to test it without having to change the global default: +When adding more worker groups, you might also want to make them the default for a specific project. This will allow you to test it without having to change the global default:🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: A comma might be missing here.
Context: ... worker groups When adding more worker groups you might also want to make them the de...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
apps/webapp/app/v3/services/worker/workerGroupService.server.ts (1)
50-54: Consider extracting flag key to a constantThe string "defaultWorkerInstanceGroupId" is used both here and on line 59. Consider extracting this to a constant to avoid string duplication and make future changes easier.
+ private readonly DEFAULT_WORKER_GROUP_FLAG_KEY = "defaultWorkerInstanceGroupId"; async createWorkerGroup({ // ...existing code - const defaultWorkerInstanceGroupId = await getFlag({ - key: "defaultWorkerInstanceGroupId", - }); + const defaultWorkerInstanceGroupId = await getFlag({ + key: this.DEFAULT_WORKER_GROUP_FLAG_KEY, + }); // ...and then later - key: "defaultWorkerInstanceGroupId", + key: this.DEFAULT_WORKER_GROUP_FLAG_KEY,apps/supervisor/src/index.ts (1)
350-350: Consider using consistent order in stop methodThe stop method orders services differently than the start method. For consistency and to ensure proper cleanup, consider keeping the same order between start and stop methods, with
workerSessionbeing last to stop after all dependent services.- await this.workerSession.stop(); - - // Optional services - await this.podCleaner?.stop(); - await this.failedPodHandler?.stop(); - await this.metricsServer?.stop(); + // Optional services + await this.podCleaner?.stop(); + await this.failedPodHandler?.stop(); + await this.metricsServer?.stop(); + + await this.workerSession.stop();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/supervisor/README.md(2 hunks)apps/supervisor/src/env.ts(4 hunks)apps/supervisor/src/index.ts(6 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)apps/supervisor/src/workloadManager/types.ts(1 hunks)apps/supervisor/src/workloadServer/index.ts(2 hunks)apps/webapp/app/routes/admin.api.v1.workers.ts(2 hunks)apps/webapp/app/routes/engine.v1.worker-actions.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.restore.ts(0 hunks)apps/webapp/app/routes/engine.v1.worker-actions.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.suspend.ts(0 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/worker/workerGroupService.server.ts(1 hunks)packages/cli-v3/src/entryPoints/managed-run-controller.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/webapp/app/routes/engine.v1.worker-actions.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.suspend.ts
- apps/webapp/app/routes/engine.v1.worker-actions.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.restore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/supervisor/src/env.ts
🧰 Additional context used
🧬 Code Definitions (2)
apps/supervisor/src/index.ts (6)
packages/core/src/v3/serverOnly/httpServer.ts (1)
HttpServer(64-367)apps/supervisor/src/env.ts (1)
env(76-76)apps/supervisor/src/services/podCleaner.ts (1)
PodCleaner(16-118)apps/supervisor/src/services/failedPodHandler.ts (1)
FailedPodHandler(18-281)apps/supervisor/src/clients/kubernetes.ts (1)
createK8sApi(9-29)apps/supervisor/src/workloadServer/index.ts (1)
WorkloadServer(62-582)
apps/supervisor/src/workloadServer/index.ts (1)
packages/core/src/v3/serverOnly/httpServer.ts (1)
HttpServer(64-367)
🪛 LanguageTool
apps/supervisor/README.md
[uncategorized] ~48-~48: A comma might be missing here.
Context: ... worker groups When adding more worker groups you might also want to make them the de...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (20)
apps/supervisor/src/workloadManager/types.ts (1)
8-8: Good addition for Kubernetes configuration flexibilityThe new
imagePullSecretsoptional property allows specifying credentials for pulling from private container registries in Kubernetes deployments. This is a valuable enhancement for enterprise environments where private registries are common.apps/supervisor/README.md (3)
11-12: Simplification of instruction textGood clarity improvement by changing from plural "edit these" to singular "edit this" since there's only one value to modify.
19-19: Improved JSON payload simplificationThe simplified JSON payload with only the
namefield makes the API easier to use. This reflects the backend changes that streamlined the worker creation process.
63-66: Good documentation for project-specific worker groupsThe added section for creating additional worker groups with project-specific defaults is well-documented and aligns with the API changes. The
makeDefaultForProjectIdparameter provides a cleaner way to set project-specific worker groups.apps/supervisor/src/workloadManager/kubernetes.ts (2)
185-187: Well-implemented method for image pull secretsThe new
getImagePullSecretsmethod cleanly extracts the logic for retrieving image pull secrets, making the code more maintainable. The implementation correctly maps string names to Kubernetes V1LocalObjectReference objects.
193-193: Good refactoring to use dynamic configurationReplacing hardcoded image pull secrets with a call to the new method improves configurability, allowing for dynamic specification of image pull secrets through options rather than hardcoding them.
apps/webapp/app/v3/services/worker/workerGroupService.server.ts (1)
50-56: Improved default worker group selection logicThe new approach of checking for an existing global default worker group is more robust than the previous count-based check. This prevents potential issues if worker groups were previously created and deleted.
packages/cli-v3/src/entryPoints/managed-run-controller.ts (1)
488-489: Enhanced execution state handlingAdding "QUEUED_EXECUTING" to the case statement ensures that runs in this state receive the same treatment as those in "EXECUTING_WITH_WAITPOINTS", which includes cleanup, debounce waiting, and suspension logic. This creates consistent handling for closely related execution states.
apps/supervisor/src/workloadServer/index.ts (2)
25-25: Metrics support integrationAdding the import for the
registerfunction enables metrics collection in the application.
125-137: Added metrics configuration and health check endpointThe changes enhance the server's monitoring capabilities in two important ways:
- Metrics collection is now configured with
registerwhile keeping metrics exposure disabled- A new health check endpoint provides a simple way to verify the server is operational
These improvements are important for production monitoring and reliability.
apps/webapp/app/routes/admin.api.v1.workers.ts (2)
10-10: API schema simplificationConsolidating the separate
projectIdandmakeDefaultfields into a singlemakeDefaultForProjectIdfield simplifies the API schema while maintaining the intended functionality.
37-38: Implementation of the consolidated schema fieldThe request handling has been properly updated to use the new consolidated field. This refactoring makes the code more maintainable by reducing the number of parameters and simplifying the conditional logic.
Also applies to: 45-49
apps/webapp/app/v3/services/initializeDeployment.server.ts (2)
21-24: Clear rejection of unsupported deployment typesThe explicit check for "UNMANAGED" deployments provides clearer error messaging when rejecting unsupported deployment types.
25-35: Automatic project engine upgrade for MANAGED deploymentsThis improvement automatically upgrades projects from engine "V1" to "V2" when a MANAGED deployment is requested. This is an excellent enhancement that reduces friction when users deploy to V2 without having run the development setup first.
apps/supervisor/src/index.ts (6)
33-33: Clear property renaming and optionality improvementThe property has been renamed from
httpServertometricsServerwith optional typing, which better reflects its purpose and accurately represents that this server may not always be initialized. This change improves code clarity and aligns with the conditional initialization later in the code.
47-51: Good security practice in debug loggingExcellent approach to debug logging by explicitly removing sensitive information (
TRIGGER_WORKER_TOKENandMANAGED_WORKER_SECRET) before logging environment variables. This prevents accidental exposure of credentials in logs.
64-92: Enhanced service initialization with improved loggingThe conditional initialization of
podCleanerandfailedPodHandlerhas been significantly improved with:
- Clear logging of configuration parameters when enabled
- Explicit warning messages when disabled
- Consistent structure for both services
This makes the system's state more transparent during startup and helps with debugging deployment configurations.
249-258: Proper conditional metrics server initializationThe metrics server is now conditionally initialized based on the
METRICS_ENABLEDenvironment variable, which aligns with the property rename and makes the metrics collection optional. The configuration is complete with appropriate port, host and register parameters.
263-263: Improved configuration flexibility for workload serverAdding the host parameter from environment variables increases deployment flexibility, allowing the workload server to bind to specific interfaces as needed.
330-332: Cleaner service management with optional chainingThe use of optional chaining (
?.) for starting and stopping services is a more elegant approach than explicit existence checks. This simplifies the code while maintaining the same behavior, properly handling cases where services might not be initialized.Also applies to: 353-355
More logging and configurable keepalive. Also includes fixes around initial worker setup.
Summary by CodeRabbit
Summary by CodeRabbit
This release introduces enhanced configuration options and connection handling flexibility for smoother operations:
Enjoy these new capabilities that aim to improve system responsiveness and operational transparency.