Skip to content

[autoscaler] Create Docker Command Runner#8806

Merged
richardliaw merged 6 commits intoray-project:masterfrom
ijrsvt:docker-command-runner
Jun 6, 2020
Merged

[autoscaler] Create Docker Command Runner#8806
richardliaw merged 6 commits intoray-project:masterfrom
ijrsvt:docker-command-runner

Conversation

@ijrsvt
Copy link
Copy Markdown
Contributor

@ijrsvt ijrsvt commented Jun 5, 2020 •
edited
Loading

Why are these changes needed?

This PR is the first in a few PRs to refactor how the autoscaler interfaces with docker. This PR mainly focuses on how the CommandRunner is instantiated. The DockerCommandRunner mostly passes commands down to an enclosed SSHCommandRunner. The main functionality change
is that rsync actually sends the file into docker.

Related issue number

Closes #4183
Closes #4403
Replaces #8527

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/latest/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested (please justify below)

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jun 5, 2020

Can one of the admins verify this patch?

ijrsvt
ijrsvt commented Jun 5, 2020
View reviewed changes
python/ray/autoscaler/updater.py
process_runner=subprocess,
use_internal_ip=False):
use_internal_ip=False,
docker_config=None):
Copy link
Copy Markdown
Contributor Author

@ijrsvt ijrsvt Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just pass the entire config in?
Currently provider_config, auth_config, cluster_name, and file_mounts (and now docker_config) are all parts

Copy link
Copy Markdown
Contributor

@richardliaw richardliaw Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's pass in the docker_config like what you're doing now

Copy link
Copy Markdown
Contributor Author

@ijrsvt ijrsvt Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay--is there a specific reason for this?

Copy link
Copy Markdown
Contributor

@richardliaw richardliaw Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it helps isolate what is needed and what is not needed. For example, initialization_commands is also part of the config, but there are times when we use the NodeUpdater where we want to run it and when we don't want to.

@ijrsvt ijrsvt requested review from ericl, richardliaw and simon-mo June 5, 2020 20:40
@richardliaw richardliaw self-assigned this Jun 5, 2020
richardliaw
richardliaw reviewed Jun 5, 2020
View reviewed changes
richardliaw
richardliaw reviewed Jun 5, 2020
View reviewed changes
richardliaw
richardliaw reviewed Jun 5, 2020
View reviewed changes
ijrsvt and others added 2 commits June 5, 2020 14:04
@ijrsvt @richardliaw
Update python/ray/autoscaler/updater.py
30f453a 
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
@ijrsvt
Small fixes
a691580
ericl
ericl reviewed Jun 5, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice!

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jun 5, 2020

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/26809/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jun 5, 2020

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/26813/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jun 6, 2020

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/26812/
Test PASSed.

@richardliaw
Copy link
Copy Markdown
Contributor

richardliaw commented Jun 6, 2020

Is this ready to merge?

@ijrsvt
Copy link
Copy Markdown
Contributor Author

ijrsvt commented Jun 6, 2020

@richardliaw Yes it is!

@richardliaw richardliaw merged commit 54189bc into ray-project:master Jun 6, 2020
ijrsvt added a commit that referenced this pull request Jun 6, 2020
@ijrsvt
Revert "[autoscaler] Create Docker Command Runner (#8806)"
f6aa87d 
This reverts commit 54189bc.
This was referenced Jun 6, 2020
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericl ericl ericl left review comments

@richardliaw richardliaw richardliaw approved these changes

@simon-mo simon-mo Awaiting requested review from simon-mo

Assignees

@richardliaw richardliaw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[tune/autoscaler] _LogSyncer cannot rsync with Docker [autoscaler] Ray submit with docker doesn't rsync the script to the container

4 participants

@ijrsvt @AmplabJenkins @richardliaw @ericl