feat: add constraints check for job resource allocation#1270
feat: add constraints check for job resource allocation#1270
Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This pull request introduces a new feature to enforce cross-resource constraints within the compute environments, significantly enhancing the fair usage enforcement capabilities. It updates the compute environment configuration to allow defining resource relationships (e.g., minimum RAM per CPU). The changes include modifications to type definitions, configuration schemas, core compute engine logic, and robust unit tests covering various constraint scenarios. Additionally, it addresses minor async-related issues in integration tests by adding await keywords.
Comments:
• [INFO][style] This is a great, comprehensive example of how to use the new cross-resource constraints. It's very helpful for understanding the feature. Perhaps a small note that gpu can be omitted if not applicable, as not all environments will have GPUs.
• [INFO][other] The ComputeResourceType union type currently includes any. While this specific change doesn't introduce it, it might be beneficial in the future to narrow down any to a more specific type or a string literal union if custom resources are enumerable, to enhance type safety.
• [INFO][performance] Calling this.checkResourceConstraints inside checkAndFillMissingResources makes sense for ensuring constraints are met after initial resource requests are processed. The iteration counts (envResources, constraints) should be small enough not to impact performance significantly.
• [INFO][other] The logic for constrainedMaxMin correctly retrieves the maximum allowed amount of the constrained resource in the current environment. This is crucial for correctly validating that a calculated requiredMin doesn't exceed the overall environment capacity for that resource. The error message is clear and helpful.
• [INFO][other] The setResourceAmount function modifies the resources array in place. This is an efficient way to adjust the requested resources, especially when bumping min values. It's good that it's a dedicated helper method.
• [INFO][bug] Adding await to oceanNode.addC2DEngines() and indexer.stopAllChainIndexers() is a good fix. It correctly handles these as asynchronous operations, preventing potential race conditions or unhandled promise rejections.
• [INFO][other] The TestC2DEngine mock and makeEnv helper are well-designed, enabling thorough and isolated testing of the constraint logic. This makes the unit tests very readable and effective.
• [INFO][other] Excellent unit test coverage for checkAndFillMissingResources and checkIfResourcesAreAvailable. The test cases cover exact matches, auto-bumping minimums, error cases for exceeding maximums, and handling of zero-amount resources (like GPU). This provides a high degree of confidence in the new constraint logic.
• [INFO][other] The constraints field is correctly marked as optional() in the Zod schema, ensuring backward compatibility with existing compute environment configurations. This is a good design choice.
Fixes # .
Changes proposed in this PR:
ResourceConstrainttype that allows compute environment resources to define min/max ratios relative to other resources (e.g. "each GPU requires at least 8 GB of RAM")checkAndFillMissingResourcesmethod, bumping dependent resources to their required minimum or throwing if the maximum is exceeded