Misc v4 checkpoint fixes #1859
Misc v4 checkpoint fixes #1859
Conversation
|
WalkthroughThis pull request refactors environment variable handling by removing an inline boolean conversion function and introducing a shared utility for parsing booleans and additional key-value environment variables. It also consolidates workload manager configuration options into a single object, updating both Docker and Kubernetes implementations. Additionally, feature flag references in the webapp have been standardized, the database schema has been updated to allow a nullable image reference, and runtime selection logic has been modified. Schema validations also reflect these changes for environment and workload configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Utility
participant Supervisor as Managed Supervisor
participant WM as Workload Manager
participant Container as Docker/Kubernetes Container
Supervisor->>Env: Request boolean and env var parsing
Env-->>Supervisor: Return parsed BoolEnv and AdditionalEnvVars
Supervisor->>WM: Construct WorkloadManagerOptions (heartbeat, snapshot intervals, additional vars)
WM->>Container: Pass environment configurations
Container-->>WM: Launch container with updated env variables
Possibly related PRs
Suggested reviewers
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 (1)
packages/cli-v3/src/config.ts (1)
176-176: Change addresses Node.js 22 compatibility issue.This change modifies the default runtime from what was likely "node-22" to the more generic "node" when run_engine_v2 feature flag is enabled. This aligns with the PR objective of addressing issues encountered with Node.js 22 by temporarily reverting to an earlier version.
Consider adding a comment explaining this change is temporary while investigating Node.js 22 issues:
- const defaultRuntime: BuildRuntime = features.run_engine_v2 ? "node" : DEFAULT_RUNTIME; + // Temporarily using generic node runtime instead of node-22 due to checkpoint compatibility issues + const defaultRuntime: BuildRuntime = features.run_engine_v2 ? "node" : DEFAULT_RUNTIME;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/supervisor/src/env.ts(2 hunks)apps/supervisor/src/envUtil.test.ts(1 hunks)apps/supervisor/src/envUtil.ts(1 hunks)apps/supervisor/src/index.ts(3 hunks)apps/supervisor/src/workloadManager/docker.ts(1 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)apps/supervisor/src/workloadManager/types.ts(1 hunks)apps/webapp/app/v3/featureFlags.server.ts(1 hunks)apps/webapp/app/v3/services/worker/workerGroupService.server.ts(3 hunks)internal-packages/database/prisma/migrations/20250331105838_make_checkpoint_image_ref_optional/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)packages/cli-v3/src/config.ts(1 hunks)packages/core/src/v3/schemas/runEngine.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
apps/webapp/app/v3/services/worker/workerGroupService.server.ts (1)
apps/webapp/app/v3/featureFlags.server.ts (2)
FEATURE_FLAG(4-6)makeSetFlags(38-55)
apps/supervisor/src/envUtil.test.ts (1)
apps/supervisor/src/envUtil.ts (2)
BoolEnv(3-9)AdditionalEnvVars(11-39)
apps/supervisor/src/env.ts (1)
apps/supervisor/src/envUtil.ts (1)
AdditionalEnvVars(11-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (23)
apps/webapp/app/v3/featureFlags.server.ts (2)
4-6: Good addition of a centralized feature flag constantCreating a centralized
FEATURE_FLAGconstant is a great practice that improves maintainability by following the DRY principle. This makes it easier to add, modify, or reference feature flags throughout the codebase while reducing the risk of typos in string literals.
9-9: Consistent usage of feature flag referenceGood update to use the constant reference instead of a string literal. This ensures consistency and makes future refactoring easier.
apps/webapp/app/v3/services/worker/workerGroupService.server.ts (4)
5-5: Good import of the feature flag constantProperly importing the
FEATURE_FLAGconstant from the feature flags module ensures consistent usage across the codebase.
52-53: Consistent use of feature flag constantReplacing the string literal with the constant reference improves maintainability and reduces the risk of typos. Good refactoring.
59-60: Consistent use of feature flag constantGood use of the constant reference here as well, maintaining consistency with the other usage.
172-173: Consistent use of feature flag constantGood final update to use the constant reference. All instances of this feature flag reference have been properly updated throughout the file.
internal-packages/database/prisma/migrations/20250331105838_make_checkpoint_image_ref_optional/migration.sql (1)
1-2: LGTM! This migration makes the imageRef field optional.The SQL migration correctly alters the "TaskRunCheckpoint" table to drop the NOT NULL constraint from the "imageRef" column, making it optional as intended in the PR objectives.
internal-packages/database/prisma/schema.prisma (1)
2067-2067: LGTM! Prisma schema change matches the migration.The change to make the
imageReffield nullable in the Prisma schema is consistent with the SQL migration and properly implements the requirement for optional image references on theTaskRunCheckpointmodel.packages/core/src/v3/schemas/runEngine.ts (2)
164-164: Schema validation updated to allow nullable imageRef.The CheckpointInput Zod schema has been correctly updated to make the imageRef field optional, which aligns with the database changes.
220-220: Schema validation updated to allow nullable imageRef for dequeued messages.The DequeueMessageCheckpoint Zod schema has been correctly updated to make the imageRef field optional. This ensures consistent validation throughout the application for the checkpoint image reference.
apps/supervisor/src/workloadManager/types.ts (1)
9-11: Well-structured extension of the WorkloadManagerOptions interface.The newly added optional properties (
heartbeatIntervalSeconds,snapshotPollIntervalSeconds, andadditionalEnvVars) enhance configurability for workloads, aligning with the PR's objective of improving environment variable configuration for runners.apps/supervisor/src/envUtil.test.ts (2)
1-36: Thorough unit tests for BoolEnv utility.The tests comprehensively cover various input scenarios for boolean parsing, including different string representations of "true" and "false", case sensitivity, whitespace handling, direct boolean values, and invalid inputs.
37-80: Comprehensive test coverage for AdditionalEnvVars utility.The tests for AdditionalEnvVars thoroughly validate parsing of key-value pairs from strings, handling whitespace, managing invalid formats, skipping invalid pairs, and handling edge cases like empty values. The test coverage ensures robust parsing of additional environment variables, which aligns with the PR objective of configuring more environment variables for runners.
apps/supervisor/src/workloadManager/docker.ts (1)
48-64: Well-implemented conditional addition of environment variables.The code properly checks for the presence of the optional configuration values before adding them to the Docker container environment. This implementation is consistent with the existing pattern and aligns with the PR objective of configuring more environment variables for runners.
apps/supervisor/src/workloadManager/kubernetes.ts (1)
137-158: Consistent implementation of environment variables in Kubernetes workload manager.The code correctly implements the same optional environment variables as in the Docker workload manager, maintaining consistency across different workload managers. The spread operator with conditional expressions is a clean approach for adding optional environment variables to the existing array.
apps/supervisor/src/envUtil.ts (2)
3-9: Well-implemented environment variable boolean parser.The BoolEnv implementation correctly handles common conventions for boolean values in environment variables, accepting both "true" and "1" (case-insensitive) as true values.
11-39: Good implementation for parsing additional environment variables.The AdditionalEnvVars utility effectively parses comma-separated key-value pairs from environment variables with proper error handling and edge case management. The implementation:
- Properly handles non-string or empty inputs
- Gracefully skips invalid entries
- Provides helpful warning logs on parsing failures
- Returns undefined when appropriate
Note that if the same key appears multiple times in the input string, the last occurrence will take precedence.
apps/supervisor/src/env.ts (2)
4-4: Good modularization of environment variable utilities.Importing the utilities from a dedicated module improves code organization and reusability.
26-29: Enhanced runner configuration with additional environment variables support.The new configuration options increase the flexibility of the runner settings:
RUNNER_HEARTBEAT_INTERVAL_SECONDSallows configuring the interval for heartbeat operationsRUNNER_SNAPSHOT_POLL_INTERVAL_SECONDSprovides control over snapshot polling frequencyRUNNER_ADDITIONAL_ENV_VARSenables passing custom environment variables to runnersThese additions align with the PR objective of allowing more environment variables for new runners.
apps/supervisor/src/index.ts (4)
5-5: Appropriate type import for the workload manager options.Importing the WorkloadManagerOptions type provides better type safety for the options object.
59-69: Good consolidation of workload manager configuration.Consolidating the configuration into a single options object improves code organization and maintainability. The use of
satisfiesensures type safety while providing better IDE support.The addition of the new runner configuration options (heartbeatIntervalSeconds, snapshotPollIntervalSeconds, and additionalEnvVars) successfully implements the PR objective of configuring more environment variables for new runners.
102-102: Simplified Kubernetes workload manager initialization.Using the consolidated options object creates cleaner, more maintainable code.
105-105: Simplified Docker workload manager initialization.Using the consolidated options object creates cleaner, more maintainable code.
Biggest change: image refs are now optional on TaskRunCheckpoint - we'll default to the deploy image
Other changes:
Summary by CodeRabbit