Fix autoscaler recovery docker config to use node-specific settings #53992
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: JohnsonKuan <johnson.h.kuan@gmail.com>
Signed-off-by: JohnsonKuan <johnson.h.kuan@gmail.com>
|
@rueian could you review this one? |
rueian
reviewed
Jun 22, 2025
| @@ -1261,7 +1261,7 @@ def recover_if_needed(self, node_id, now): | |||
| process_runner=self.process_runner, | |||
| use_internal_ip=True, | |||
| is_head_node=False, | |||
| docker_config=self.config.get("docker"), | |||
| docker_config=self._get_node_specific_docker_config(node_id), | |||
There was a problem hiding this comment.
@rueian could you review this one?
Sure. Hi @JohnsonKuan, the change of using _get_node_specific_docker_config looks good to me.
def _get_node_specific_docker_config(self, node_id):
if "docker" not in self.config:
return {}
docker_config = copy.deepcopy(self.config.get("docker", {}))
node_specific_docker = self._get_node_type_specific_fields(node_id, "docker")
docker_config.update(node_specific_docker)
return docker_configI think using _get_node_specific_docker_config is correct and pretty safe. However, can we still have a test in test_autoscaler.py? I think we could use runner.assert_has_call or runner.command_history to test which image it uses.
There was a problem hiding this comment.
Thanks @rueian! Yes, absolutely. I’ve added a test in test_autoscaler.py as suggested using runner.assert_has_call to verify the correct image is used.
There was a problem hiding this comment.
Hi @jjyao, this PR is ready to be merged! PTAL.
…during recovery Signed-off-by: JohnsonKuan <johnson.h.kuan@gmail.com>
rueian
approved these changes
Jun 23, 2025
minerharry
pushed a commit
to minerharry/ray
that referenced
this pull request
Jun 27, 2025
…ay-project#53992) The autoscaler's `recover_if_needed` method currently uses `self.config.get("docker")` which only retrieves the top-level docker configuration. However, regular node updates use `self._get_node_specific_docker_config(node_id)` which properly merges top-level and node-type-specific docker settings. This inconsistency means that when a node type has specific docker requirements (different image, volumes, environment variables, etc.), recovery attempts will ignore those node-type-specific settings and only use the global docker configuration. This could cause recovery to fail or behave differently than the original node setup. For example, if a node type is configured with a specific docker image or custom volumes, the recovery process will not use those settings, potentially leading to recovery failures or inconsistent behavior between initial node setup and recovery attempts. The fix ensures that recovery attempts use the same docker configuration logic as regular node updates, maintaining consistency and ensuring that node-type-specific docker settings are properly applied during recovery. Closes ray-project#53987 --------- Signed-off-by: JohnsonKuan <johnson.h.kuan@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
The autoscaler's
recover_if_neededmethod currently usesself.config.get("docker")which only retrieves the top-level docker configuration. However, regular node updates useself._get_node_specific_docker_config(node_id)which properly merges top-level and node-type-specific docker settings.This inconsistency means that when a node type has specific docker requirements (different image, volumes, environment variables, etc.), recovery attempts will ignore those node-type-specific settings and only use the global docker configuration. This could cause recovery to fail or behave differently than the original node setup.
For example, if a node type is configured with a specific docker image or custom volumes, the recovery process will not use those settings, potentially leading to recovery failures or inconsistent behavior between initial node setup and recovery attempts.
The fix ensures that recovery attempts use the same docker configuration logic as regular node updates, maintaining consistency and ensuring that node-type-specific docker settings are properly applied during recovery.
Related issue number
Closes #53987
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.