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

Only keep :latest tag for local images #88

Merged
merged 1 commit into from
May 30, 2021
Merged

Conversation

andreasjansson
Copy link
Member

@andreasjansson andreasjansson commented May 21, 2021

Images are built, tagged with registry and content-addressable tag,
pushed, re-tagged as :latest, and the original tag is removed.

This prevents Cog from using a ton of disk space, but builds are still
fast since the cached layers from the last build are still kept around
thanks to the :latest tag.

ref #80 #18

Signed-off-by: andreasjansson andreas@replicate.ai

Images are built, tagged with registry and content-addressable tag,
pushed, re-tagged as :latest, and the original tag is removed.

This prevents Cog from using a ton of disk space, but builds are still
fast since the cached layers from the last build are still kept around
thanks to the :latest tag.

Signed-off-by: andreasjansson <andreas@replicate.ai>
@bfirsh
Copy link
Member

bfirsh commented May 27, 2021

Unfortunately I don't think this fixes the issue. When you update a tag, the image the tag previously pointed at is not automatically removed.

I tried this locally and the previous image does indeed remain when running a second build:

Screen Shot 2021-05-27 at 13 34 45

But -- this does at least allow it to be freed up by docker image prune, which is an improvement over the previous situation.

As I suggested in #80 (but wasn't fully clear about), I think a pointer to the previous image needs to be kept so that when the latest tag is updated, the previous image is also removed.

There's a race condition in here if two images are being built at the same time. Perhaps when removing the image, if it fails it should be ignored.

@andreasjansson
Copy link
Member Author

Hmm that's odd, there's a test case for exactly the situation you're describing here that checks that only the ":latest" tag is left after a build. This PR doesn't just re-tag, it also removes the old image with docker rmi.

I don't think there's a race here, if two versions in the same model but with different IDs are built simultaneously, one of them will end up being tagged as :latest, and they'll both get pushed.

The :latest tag is only kept around for caching purposes. Without it, the cached layers are removed when you docker rmi the tagged image.

@bfirsh
Copy link
Member

bfirsh commented May 28, 2021

(I noticed another bit of odd behavior in here. The Docker image is being tagged with its id. This is a bit unusual and I think this is creating some confusion about what's going on here. I've prefixed the image IDs with sha256: below for clarity.)

Here is what is happening:

  1. New version 5c334ebb90a5c54f9f99bf281af2ad283d3ae0d7 is pushed, image sha256:d464d56f9e82 is created with tags latest and d464d56f9e82.
  2. Tag d464d56f9e82 is pushed
  3. Tag d464d56f9e82 is removed.

It now looks like this:

Screen Shot 2021-05-28 at 08 42 12

Now, I push a new version:

  1. New version 704527cf09078ee4b0c33a0d9e23b9de30ec8240 is pushed, image sha256:5b0c5630ff2c is created and tagged with d464d56f9e82
  2. Image sha256:5b0c5630ff2c is tagged with latest which implicitly removes the tag from previous image sha256:d464d56f9e82, but does not remove the image. Image sha256:d464d56f9e82 is now dangling without a tag.
  3. Tag d464d56f9e82 is pushed
  4. Tag d464d56f9e82 is removed.

It now looks like this:

Screen Shot 2021-05-28 at 08 44 17

The previous image still exists and won't be cleaned up until I removed it with docker rmi d464d56f9e82 or docker image prune.

Hmm that's odd, there's a test case for exactly the situation you're describing here that checks that only the ":latest" tag is left after a build. This PR doesn't just re-tag, it also removes the old image with docker rmi.

The test only tests when an image is pushed the first time.

I don't think there's a race here, if two versions in the same model but with different IDs are built simultaneously, one of them will end up being tagged as :latest, and they'll both get pushed.

Sorry to be clear: the race condition exists in the behavior that is probably needed to fix this problem, where a pointer is kept to an image. That image might be removed twice if a push is happening in parallel.

As mentioned above, Docker images are being tagged with their ID, which makes Docker behave a bit weirdly (tags and IDs are ambiguous in the user interface). Perhaps we should tag them with their build ID?

@bfirsh
Copy link
Member

bfirsh commented May 30, 2021

I'm going to get this in because then at least docker image prune works. And, I figure cleaning up the images is an additional piece of work on top of this, not a completely different implementation. Will consider #18 not fixed though until disk space is actually cleared.

@bfirsh bfirsh merged commit e85f8eb into main May 30, 2021
@bfirsh bfirsh deleted the andreas/image-cleanup branch May 30, 2021 21:51
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit to bfirsh/cog that referenced this pull request May 31, 2021
This is causing some confusion in Docker behavior.

See replicate#88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit that referenced this pull request Jun 1, 2021
This is causing some confusion in Docker behavior.

See #88 (comment)

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
andreasjansson added a commit that referenced this pull request Jun 8, 2021
Following up from #88, this will
keep Cog from leaving dangling Docker images that take lots of disk
space.

Signed-off-by: andreasjansson <andreas@replicate.ai>
bfirsh pushed a commit that referenced this pull request Jun 8, 2021
Following up from #88, this will
keep Cog from leaving dangling Docker images that take lots of disk
space.

Signed-off-by: andreasjansson <andreas@replicate.ai>
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.

None yet

2 participants