Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Fix docker in image_id #3481

Merged
merged 4 commits into from
Apr 26, 2024
Merged

[Core] Fix docker in image_id #3481

merged 4 commits into from
Apr 26, 2024

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Apr 25, 2024

Fixes issue introduced in #3436, as some cloud's default images do not have /etc/docker/daemon.json by default, which causes the following error, during initializing docker image

E 04-25 19:02:45 subprocess_utils.py:84] jq: error: Could not open file /etc/docker/daemon.json: No such file or directory
E 04-25 19:02:45 subprocess_utils.py:84] Job for docker.service failed because the control process exited with error code.
E 04-25 19:02:45 subprocess_utils.py:84] See "systemctl status docker.service" and "journalctl -xe" for details.
E 04-25 19:02:45 subprocess_utils.py:84] 
E 04-25 19:02:54 subprocess_utils.py:84] Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky launch -c test-docker --cloud gcp --image-id docker:continuumio/miniconda3:24.1.2-0 echo hi
  • All smoke tests: pytest tests/test_smoke.py
    • pytest tests/test_smoke.py::test_docker_storage_mounts tests/test_smoke.py::test_docker_preinstalled_package --gcp
    • pytest tests/test_smoke.py::test_docker_storage_mounts tests/test_smoke.py::test_docker_preinstalled_package --aws
    • pytest tests/test_smoke.py::test_docker_storage_mounts tests/test_smoke.py::test_docker_preinstalled_package --azure
    • pytest tests/test_smoke.py::test_docker_storage_mounts tests/test_smoke.py::test_docker_preinstalled_package --fluidstack
    • pytest tests/test_smoke.py::test_docker_storage_mounts --kubernetes
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@Michaelvll Michaelvll changed the title [Core] Fix docker for GCP [Core] Fix docker in image_id Apr 25, 2024
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for identifying and fixing this @Michaelvll ! Left some comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we fix that for node providers as well?

self.run(
'sudo jq \'.["exec-opts"] = ["native.cgroupdriver=cgroupfs"]\' '
'/etc/docker/daemon.json > /tmp/daemon.json;'
'sudo mv /tmp/daemon.json /etc/docker/daemon.json;'
'sudo systemctl restart docker',
run_env='host')

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this fix does not works for k8s?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is unrelated to the test_docker_preinstalled_package for k8s. That one is probably because we did not install the required packages in the docker container.

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! LGTM.

@Michaelvll Michaelvll merged commit d0a1a86 into master Apr 26, 2024
20 checks passed
@Michaelvll Michaelvll deleted the fix-docker-gcp branch April 26, 2024 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants