Optimize docker initialization #3823
Conversation
There was a problem hiding this comment.
Thanks for submitting this PR @camelop! This is awesome. Left several comments. : )
sky/provision/docker_utils.py
Outdated
| if [[ $docker_installed -eq 1 ]] | ||
| then | ||
| # wait for docker daemon to be ready | ||
| docker_ready=0 |
There was a problem hiding this comment.
How about renaming this to docker_socket_ready?
|
|
||
| # SkyPilot: Docker login if user specified a private docker registry. | ||
| cmd_login = '' | ||
| if 'docker_login_config' in self.docker_config: |
There was a problem hiding this comment.
Can we combine the following into the same call as the _check_host_status?
There was a problem hiding this comment.
Also, I am wondering as we may get a longer commands to run, will it be more readable if we explicitly create a enum as a state machine to indicate the stage we are in, and we can do post processing based on the state.
There was a problem hiding this comment.
It's combined to the next "action" call.
Currently it is following the structure of
- remote read-only status checking with sh
- local operation decision making in Python
- remote action
To further save SSH calls, I would recommend completely refactor the execution structure into the following:
- gather all local Python execution demands and render an execution script with it (e.g. with templating from Jinja2, similar to https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_templating.html ). The script should potentially execute different branches depending on status on the target machine.
- once the script is rendered ready, send it to remote for execution
- gather the result in one round to summarize whether the change has been successfully replied
I would expect such change to be a global coding style / design pattern change, so not sure if it is a good fit for a minor opt PR like this.
| if 'container_status' in status: | ||
| if 'Exited' in status['container_status']: | ||
| logger.info('Container is exited but not removed.') | ||
| self.initialized = True | ||
| self._run(f'{self.docker_cmd} start {self.container_name}') | ||
| self._run('sudo service ssh start', run_env='docker') | ||
| return self._run('whoami', run_env='docker') |
There was a problem hiding this comment.
Can this be integrated into the check_host_status command as well?
There was a problem hiding this comment.
This one is not on the happy path though, but also follow the above design pattern (the action part in step 3).
Reduce ssh interaction round number on the happy path from 14 to 5 for docker initialization.
Related issue: #3661
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_docker_preinstalled_packageconda deactivate; bash -i tests/backward_compatibility_tests.sh