[Docker] Add retry for docker pull due to daemon not ready #3218

Michaelvll · 2024-02-23T07:17:45Z

pytest tests/test_smoke.py::test_job_queue_with_docker --azure fails because ray cluster fail to start on the remote machine. Logs: https://gist.github.com/Michaelvll/4528e47faa9a008e1b204ea8b6873947

The main reason is that the docker pull fails due to the docker daemon not ready when the cluster is just up, and our retry in cloud_vm_ray_backend tries ray up again with --no-restart passed, which causes the ray cluster fail to start due to some bug in ray.

skypilot/sky/backends/cloud_vm_ray_backend.py

Lines 1832 to 1843 in ad755df

    
           while (retry_cnt < _MAX_RAY_UP_RETRY and 
        
                  need_ray_up(ray_up_return_value)): 
        
               retry_cnt += 1 
        
               if retry_cnt > 1: 
        
                   sleep = backoff.current_backoff() 
        
                   logger.info( 
        
                       'Retrying launching in {:.1f} seconds.'.format(sleep)) 
        
                   time.sleep(sleep) 
        
               # TODO(zhwu): when we retry ray up, it is possible that the ray 
        
               # cluster fail to start because --no-restart flag is used. 
        
               ray_up_return_value = ray_up()

In this PR, we retry the docker pull when the cluster is just up, so it does not error out and trigger that retry.

Future TODO:

We should make the retry of ray up more robust and make sure the ray cluster is started on the cluster.
A more useful way is to move all the cloud implementation to new provisioner API

Tested (run the relevant ones):

cblmemo

Thanks! it looks mostly great to me. Left a comment on exception type

cblmemo · 2024-02-23T08:57:14Z

sky/skylet/providers/command_runner.py

+                        '1> /dev/null  2>&1 || '
+                        f'{self.docker_cmd} pull {specific_image}')
+                break
+            except click.ClickException as e:


why is it click.ClickException? Should we catch broad exception here?

It is the exception raised by the underlying command runner when the command fails. It might be a better practice to only catch the exception that is raised by the underlying function.

sky/skylet/providers/command_runner.py

cblmemo · 2024-02-24T01:11:18Z

sky/templates/gcp-ray.yml.j2

+{%- if docker_login_config is not none %}
+  docker_login_config:
+    username: {{docker_login_config.username}}
+    password: {{docker_login_config.password}}
+    server: {{docker_login_config.server}}
+{%- endif %}


Just want to make sure, we don't have gcp docker support w/ new provisioner until this PR..?

yea... the private docker support does not work for GCP and Azure before this. We should probably add a smoke test for private docker image. Could you help file an issue and look into this?

…org#3218) * Add retry for docker pull due to daemon not ready * longer wait time * longer wait time * retry earlier * add retry for retries as well * longer wait time * change wait time * format * Add comment * Fix * Fix indent for azure docker config * Fix docker login config * Fix comments * More robust docker login config * Add retry for docker check * minor fix * Add additional test for stop and start with docker * Fix cancelled

* initial commit * newline * comments * run linter * reminder for down * tentatively done with example * formatting * yapf * [Storage] Storage mounting tool permissions fix (#3215) * fix permissions * fix permissions * [LLM] Example for Serving Gemma (#3207) * Add serve for gemma and fix mixtral dependency * Add hf token * fix model len * Add comment * Serve your private gemma * fix serve yaml * readme * Remove chat completion due to the wrong template * add readme * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * address comments * Update README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Change to it * Add chat API * use HF_TOKEN env * typo --------- Co-authored-by: Zongheng Yang <[email protected]> * [LLM] Add logo for Gemma (#3220) * Minor fixes for release 0.5.0 (#3212) * when removing cudo credential, sky check fails * remove tips * minor hint fix * fix cluster version for k8s * fix typo * [Docker] Add retry for docker pull due to daemon not ready (#3218) * Add retry for docker pull due to daemon not ready * longer wait time * longer wait time * retry earlier * add retry for retries as well * longer wait time * change wait time * format * Add comment * Fix * Fix indent for azure docker config * Fix docker login config * Fix comments * More robust docker login config * Add retry for docker check * minor fix * Add additional test for stop and start with docker * Fix cancelled * added comments * quick fix * finished pip issues * fix * fix storage error message, add example link to docs --------- Co-authored-by: Sheth <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> Co-authored-by: Zhanghao Wu <[email protected]> Co-authored-by: Zongheng Yang <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]>

* initial commit * newline * comments * run linter * reminder for down * tentatively done with example * formatting * yapf * [Storage] Storage mounting tool permissions fix (#3215) * fix permissions * fix permissions * [LLM] Example for Serving Gemma (#3207) * Add serve for gemma and fix mixtral dependency * Add hf token * fix model len * Add comment * Serve your private gemma * fix serve yaml * readme * Remove chat completion due to the wrong template * add readme * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * address comments * Update README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Change to it * Add chat API * use HF_TOKEN env * typo --------- Co-authored-by: Zongheng Yang <[email protected]> * [LLM] Add logo for Gemma (#3220) * Minor fixes for release 0.5.0 (#3212) * when removing cudo credential, sky check fails * remove tips * minor hint fix * fix cluster version for k8s * fix typo * [Docker] Add retry for docker pull due to daemon not ready (#3218) * Add retry for docker pull due to daemon not ready * longer wait time * longer wait time * retry earlier * add retry for retries as well * longer wait time * change wait time * format * Add comment * Fix * Fix indent for azure docker config * Fix docker login config * Fix comments * More robust docker login config * Add retry for docker check * minor fix * Add additional test for stop and start with docker * Fix cancelled * added comments * quick fix * finished pip issues * fix * fix storage error message, add example link to docs * logging for SSH when doing kubernetes provision * romil edits * took out todo commnt * removed extra file * renamed file * restored right version of file * simplify things * newline * more formatting * formatting * minor fixes * set x and logging * fixes * docstr --------- Co-authored-by: Sheth <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> Co-authored-by: Zhanghao Wu <[email protected]> Co-authored-by: Zongheng Yang <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]>

* initial commit * newline * comments * run linter * reminder for down * tentatively done with example * formatting * yapf * [Storage] Storage mounting tool permissions fix (#3215) * fix permissions * fix permissions * [LLM] Example for Serving Gemma (#3207) * Add serve for gemma and fix mixtral dependency * Add hf token * fix model len * Add comment * Serve your private gemma * fix serve yaml * readme * Remove chat completion due to the wrong template * add readme * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * address comments * Update README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Change to it * Add chat API * use HF_TOKEN env * typo --------- Co-authored-by: Zongheng Yang <[email protected]> * [LLM] Add logo for Gemma (#3220) * Minor fixes for release 0.5.0 (#3212) * when removing cudo credential, sky check fails * remove tips * minor hint fix * fix cluster version for k8s * fix typo * [Docker] Add retry for docker pull due to daemon not ready (#3218) * Add retry for docker pull due to daemon not ready * longer wait time * longer wait time * retry earlier * add retry for retries as well * longer wait time * change wait time * format * Add comment * Fix * Fix indent for azure docker config * Fix docker login config * Fix comments * More robust docker login config * Add retry for docker check * minor fix * Add additional test for stop and start with docker * Fix cancelled * added comments * quick fix * finished pip issues * fix * fix storage error message, add example link to docs * changed error message if default nc installed on mac * refactored check_port_forward_mode_dependencies function * update comment --------- Co-authored-by: Sheth <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> Co-authored-by: Zhanghao Wu <[email protected]> Co-authored-by: Zongheng Yang <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]>

* initial commit * newline * comments * run linter * reminder for down * tentatively done with example * formatting * yapf * [Storage] Storage mounting tool permissions fix (#3215) * fix permissions * fix permissions * [LLM] Example for Serving Gemma (#3207) * Add serve for gemma and fix mixtral dependency * Add hf token * fix model len * Add comment * Serve your private gemma * fix serve yaml * readme * Remove chat completion due to the wrong template * add readme * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * address comments * Update README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Update llm/gemma/README.md Co-authored-by: Zongheng Yang <[email protected]> * Change to it * Add chat API * use HF_TOKEN env * typo --------- Co-authored-by: Zongheng Yang <[email protected]> * [LLM] Add logo for Gemma (#3220) * Minor fixes for release 0.5.0 (#3212) * when removing cudo credential, sky check fails * remove tips * minor hint fix * fix cluster version for k8s * fix typo * [Docker] Add retry for docker pull due to daemon not ready (#3218) * Add retry for docker pull due to daemon not ready * longer wait time * longer wait time * retry earlier * add retry for retries as well * longer wait time * change wait time * format * Add comment * Fix * Fix indent for azure docker config * Fix docker login config * Fix comments * More robust docker login config * Add retry for docker check * minor fix * Add additional test for stop and start with docker * Fix cancelled * added comments * quick fix * finished pip issues * fix * fix storage error message, add example link to docs * prototype for gcs * removed non git files * rclone support for aws * minor formatting fix * fixed merge conflict * fixed merge conflict * fixed merge conflict * reset perf example * reset perf example * added tests for rclone functionality * update rclone vfs options * testing * test * test * update rclone config and rclone command options * nit refactor * rclone refactor * nit * update MOUNT_CACHE mode to MOUNT_CACHED mode * update smoke test * additional comments for mount cached command explaining options for rclone vfs mode * rclone class doc-string fix * nit format * update step 7 of maybe_translate_local_file_mounts_and_sync_up * Run rclone in the background so it does not hang with rclone log * update log path * R2 support * Support Azure Blob Storage * format * minor fixes * add max cache size, correctly mount cached in managed jobs * change log to info level * wip script to check for if vfs cache is flushed * flushed out design for flush wait logic * use hashed mount path for cache/logs to avoid collision * review feedback, test just running rclone as daemon * remainder of feedback * Update sky/data/mounting_utils.py Co-authored-by: Christopher Cooper <[email protected]> * change storage test bucket name * remove cached code outside of if * only test rclone cmd when cached mount is there * add cached mount to storage md * one more test refactor * nofail * better match * use deterministic hashing * more refactor --------- Co-authored-by: Sheth <[email protected]> Co-authored-by: Hriday Sheth <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> Co-authored-by: Zhanghao Wu <[email protected]> Co-authored-by: Zongheng Yang <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> Co-authored-by: Seung Jin <[email protected]> Co-authored-by: Christopher Cooper <[email protected]>

Michaelvll added 3 commits February 23, 2024 07:12

Add retry for docker pull due to daemon not ready

ad755df

longer wait time

857945d

longer wait time

861ddf6

Michaelvll requested a review from cblmemo February 23, 2024 08:04

cblmemo approved these changes Feb 23, 2024

View reviewed changes

Michaelvll added 6 commits February 23, 2024 20:50

retry earlier

12119a4

add retry for retries as well

b6b539a

longer wait time

0587d90

change wait time

7ffc9c8

format

1708909

Add comment

40b3afd

Michaelvll mentioned this pull request Feb 23, 2024

Release 0.5.0 #3211

Merged

14 tasks

Michaelvll added 4 commits February 24, 2024 00:01

Fix

6a2a40a

Fix indent for azure docker config

ba5a6af

Fix docker login config

1ef3d92

Fix comments

a6d897d

cblmemo reviewed Feb 24, 2024

View reviewed changes

Michaelvll added 5 commits February 24, 2024 01:44

More robust docker login config

55e196b

Add retry for docker check

dbd4f4a

minor fix

6768c1a

Add additional test for stop and start with docker

08d7ef7

Fix cancelled

57fb1f3

Michaelvll merged commit c20fdab into master Feb 24, 2024

Michaelvll deleted the retry-docker-pull branch February 24, 2024 03:18

cblmemo mentioned this pull request Feb 24, 2024

[Tests] Add smoke test for private docker registry #3225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Docker] Add retry for docker pull due to daemon not ready #3218

[Docker] Add retry for docker pull due to daemon not ready #3218

Uh oh!

Michaelvll commented Feb 23, 2024 •

edited

Loading

Uh oh!

cblmemo left a comment

Uh oh!

cblmemo Feb 23, 2024 •

edited

Loading

Uh oh!

Michaelvll Feb 23, 2024

Uh oh!

Uh oh!

cblmemo Feb 24, 2024

Uh oh!

Michaelvll Feb 24, 2024 •

edited

Loading

Uh oh!

cblmemo Feb 24, 2024

Uh oh!

Uh oh!

	while (retry_cnt < _MAX_RAY_UP_RETRY and
	need_ray_up(ray_up_return_value)):
	retry_cnt += 1
	if retry_cnt > 1:
	sleep = backoff.current_backoff()
	logger.info(
	'Retrying launching in {:.1f} seconds.'.format(sleep))
	time.sleep(sleep)
	# TODO(zhwu): when we retry ray up, it is possible that the ray
	# cluster fail to start because --no-restart flag is used.
	ray_up_return_value = ray_up()

[Docker] Add retry for docker pull due to daemon not ready #3218

[Docker] Add retry for docker pull due to daemon not ready #3218

Uh oh!

Conversation

Michaelvll commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cblmemo left a comment

Choose a reason for hiding this comment

Uh oh!

cblmemo Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Michaelvll Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cblmemo Feb 24, 2024

Choose a reason for hiding this comment

Uh oh!

Michaelvll Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cblmemo Feb 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Michaelvll commented Feb 23, 2024 •

edited

Loading

cblmemo Feb 23, 2024 •

edited

Loading

Michaelvll Feb 24, 2024 •

edited

Loading