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

Check the image is outdated and pop warning to pull the latest #193

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions aiidalab_launch/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,14 +347,17 @@ async def _async_start(
with spinner(msg):
instance.pull()
else:
from aiidalab_launch import util

# use local image
msg = f"Using local image '{profile.image}'."

if instance.image is None:
raise click.ClickException(
f"Unable to find image '{profile.image}'. "
"Try to use '--pull' to pull the image prior to start."
)
# check if local image is outdated and pull latest version if so
if not util.image_is_latest(instance.client, profile.image):
click.secho(
"Warning! Local image is outdated, please run with --pull to update.",
fg="yellow",
)

# Check if the container configuration has changed.
if instance.container:
Expand Down
23 changes: 23 additions & 0 deletions aiidalab_launch/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,26 @@ def get_docker_env(container: docker.models.containers.Container, env_name: str)
except KeyError:
pass
raise KeyError(env_name)


def image_is_latest(docker_client, image: str):
"""Check if the local image has the same digest as the image
on remote registry.
"""
try:
local_image = docker_client.images.get(image)
except docker.errors.ImageNotFound:
return False

try:
remote_image = docker_client.images.get_registry_data(image)
except docker.errors.APIError:
return False

# There is no need to check creation date of the image, since the once
# there is a new image with the same tag, the id will be different.
# We can not use image id, see https://windsock.io/explaining-docker-image-ids/
local_digest = local_image.attrs.get("RepoDigests")[0].split("@")[-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little unhappy about the split()[-1] here since that does not seem very robust. But if there is not a better way of getting the digest than I guess it is fine.

remote_digest = remote_image.attrs.get("Descriptor", {}).get("digest")
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the remote_digest ends up being None here? We should probably return true in that case, since that is likely a problem with the Dockerhub and we should not pull. (or it could indicate that the API changed)


return local_digest == remote_digest
13 changes: 13 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,19 @@ def docker_client():
pytest.skip("docker not available")


@pytest.fixture(scope="function")
def remove_created_images(docker_client):
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little nervous here, how does this ensure that we do not wipe out all the images on the machine? Is that ensured by the docker_client fixture?

Even if it supposedly is, I would be much more comfortable if we filtered the image names somehow (e.g. the image name needs to contain test or some other unique string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this more, this approach is definitely not safe since, since other processes can create docker images in the meantime while the test is running. The logic here would delete these images.

Instead we could perhaps hard-code the name of the test image (`hello-world"), which should always be safe to delete.

"""Remove all images created by the tests."""
images = docker_client.images.list()
yield
for image in docker_client.images.list():
if image not in images:
try:
image.remove()
except docker.errors.APIError:
pass


@pytest.fixture(autouse=True)
def _select_default_image(monkeypatch_session, pytestconfig):
_default_image = pytestconfig.getoption("default_image")
Expand Down
16 changes: 15 additions & 1 deletion tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ def test_remove_running_profile(self):
@pytest.mark.slow
@pytest.mark.trylast
class TestInstanceLifecycle:
def test_start_stop_reset(self, instance, docker_client, caplog):
def test_start_stop_reset(self, instance, docker_client, caplog, monkeypatch):
caplog.set_level(logging.DEBUG)

def get_volume(volume_name):
Expand Down Expand Up @@ -260,6 +260,20 @@ def assert_status_down():
assert result.exit_code == 0
assert_status_up()

# test the warning message of image not the latest is not raised
assert "Warning!" not in result.output.strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, you could maybe have this assert earlier after the container is first started. Then you could move the monkeypatch above as well and avoid starting up the container for the third time.


# Then by monkeypatching the image_is_latest function, we can test that
# the warning message is raised
def image_is_latest(docker_client, image_name):
return False

monkeypatch.setattr("aiidalab_launch.util.image_is_latest", image_is_latest)
result: Result = runner.invoke(
cli.cli, ["start", "--no-browser", "--no-pull", "--wait=300"]
)
assert "Warning!" in result.output.strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you get this assert be a more specific than just "Warning"? That is too generic.
Also please also check the exit code and that the container is up and has already been running, as done above?


# Restart instance.
# TODO: This test is currently disabled, because it is too flaky. For
# a currently unknown reason, the docker client will not be able to
Expand Down
31 changes: 30 additions & 1 deletion tests/test_util.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from time import time

import pytest
from packaging.version import parse

from aiidalab_launch.util import get_latest_version
from aiidalab_launch.util import get_latest_version, image_is_latest


def test_get_latest_version(mock_pypi_request):
Expand All @@ -13,3 +14,31 @@ def test_get_latest_version_timeout(mock_pypi_request_timeout):
start = time()
assert get_latest_version() is None
assert (time() - start) < 0.5


@pytest.mark.usefixtures("enable_docker_pull")
@pytest.mark.usefixtures("remove_created_images")
def test_image_is_latest(docker_client):
"""Test that the latest version is identified correctly."""
# download the alpine image for testing
image_name = "alpine:latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use something more lightweight, perhaps the hello-world image from Docker?

Also, it is not clear to me, is this image then automatically deleted in enable_docker_pull fixture?

Copy link
Member Author

@unkcpz unkcpz Oct 6, 2023

Choose a reason for hiding this comment

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

more lightweight, perhaps the hello-world image from Docker?

👍

is this image then automatically deleted in enable_docker_pull fixture?

No, I don't think so. This fixture is to guarantee the test will be skipped if it contains docker pull.

Copy link
Member Author

Choose a reason for hiding this comment

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

What a surprise, the hello-world is bigger than alpine. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. But that seems to be only true for for windows (which does not exist for Alpine), for linux it is only a couple of kB, i.e. an order of magnitute smaller. So I would still prefer to switch to hello-world image, and mark this test as skipped if ran on windows (I don't think we support windows anyway, but just to ensure we do not end up downloading 100Mb in case anybody tried it).

Also since these tests rely on the network, I would prefer if they were marked as "slow".

docker_client.images.pull(image_name)

# check that the image is identified as latest
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to also test the converse, i.e. pull some outdated version of the same image and check that it is identified as not latest. That should be possible right? (should be a separate test to keep it clean)

Copy link
Member Author

@unkcpz unkcpz Oct 6, 2023

Choose a reason for hiding this comment

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

I think it can, by pulling the old image and retag it. See the new commit.

assert image_is_latest(docker_client, image_name)


@pytest.mark.usefixtures("enable_docker_pull")
@pytest.mark.usefixtures("remove_created_images")
def test_image_is_not_latest(docker_client):
"""Test that the outdate version is identified correctly and will ask for pull the latest."""
# download the alpine image for testing
old_image_name = "alpine:2.6"
latest_image_name = "alpine:latest"

# pull the old image and retag it as latest to mock the outdated image
old_image = docker_client.images.pull(old_image_name)
old_image.tag(latest_image_name)

# check that the image is identified as latest
assert not image_is_latest(docker_client, latest_image_name)
Loading