r-darwish / topgrade

Upgrade everything
GNU General Public License v3.0
3.36k stars 160 forks source link

Updating docker images #853

Closed raszi closed 2 years ago

raszi commented 2 years ago

I want to suggest a new step

I would like to propose a docker image update step. Since tagged images could be also updated, and should be updated because of security reasons for example.

Which tool is this about? Where is its repository?

Docker

Which operating systems are supported by this tool?

Linux, MacOS, Windows

What should Topgrade do to figure out if the tool needs to be invoked?

Check whether the docker command exists and there are images.

Which exact commands should Topgrade run?

Something like:

docker images --format "{{.Repository}}:{{.Tag}}" | xargs -n 1 docker pull

There should be a way to ignore specific images since locally tagged images could not be updated and docker exits with a non-zero status code in that case.

MCOfficer commented 2 years ago

Without drawing any kind of conclusion, there are established solutions to this problem, f.e. watchtower.

raszi commented 2 years ago

Thanks for the tip @MCOfficer. I guess there are separate solutions to update everything that topgrade supports, but the whole point of it is to have it in one place.

Funky185540 commented 2 years ago

@raszi Would you mind giving my implementation from PR #850 a try? You should be able to just:

git clone https://github.com/Funky185540/topgrade.git
git checkout feature/upgrade-podman-containers
cargo run -- --only containers --verbose --dry-run

And if the output looks reasonable to you, leave out the --dry-run. Tell me if it worked, or what you would like to have improved please!

raszi commented 2 years ago

Thank you @Funky185540 that was quick!

I've tried it out and it does work! Although it exits with an error since there are local images that could not be fetched.

Therefore - I believe - this step will fail in most of the cases since docker users have locally tagged containers at least for ones that are created by docker-compose.

Containers: FAILED
 2022-02-08T09:32:49.013Z DEBUG topgrade::terminal          > Desktop notification: Topgrade finished with errors
Funky185540 commented 2 years ago

I've tried it out and it does work! Although it exits with an error since there are local images that could not be fetched.

Humm, it should avoid that... Could you post one of the lines where it failed to pull a local container? I'm not a docker veteran, my code is based on the assumption that containers built locally have localhost as origin repository. But I can change that of course, if I know what to.

Funky185540 commented 2 years ago

Or what if we restrict it to update only containers from a selection of known registries? Then we could allow the user to specify additional registries in the topgrade config, similar to how you can already specify additional git repos that should be pulled. The way it's currently written this would also allow to specifically add foo.io/bar/baz as "registry" to pull only container bar/baz from foo.io, in cases where your other containers from foo.io need authentication because they are in private repos (Which would make the Update step fail, because docker/podman returns an error code in that case unless you are authenticated already).

What do you think about that?

raszi commented 2 years ago

Restricting the images from specific repositories sounds good, what I don't know is how to filter the images based on that. I am not sure that you can distinguish the locally tagged images from remote tagged images. You can tag an image locally which later could be tagged exactly the same in the Docker registry.

If I run a docker inspect <imageid> there seems no information about that so I believe there is no difference. I guess the only option is to handle the pulling errors gracefully.

Funky185540 commented 2 years ago

Do you mean "locally tagged images" as in you have performed changes to the image, commited them and then tagged that as container:tag locally?

Edit:

Using a different tag from what's available on the remote I assume? Like container:my-modifications.

raszi commented 2 years ago

As I wrote earlier it is not that easy. When you are using docker-compose, then it would automatically tag images like the following:

REPOSITORY            TAG                 IMAGE ID            CREATED             SIZE  
xxxxxxx-docker_node   latest              134880d0a572        6 weeks ago         956MB
xxxxxxx-docker_app    latest              085f79ee5ada        6 weeks ago         1.36GB
xxxxxxx-docker_web    latest              dc91cfe68a71        6 weeks ago         177MB

Where the xxxxxxx-docker is the name of the directory where the docker-compose.yml file is in.

raszi commented 2 years ago

Theoretically, somebody could create a Docker image in the Docker registry, with a name like xxxxxxx-docker_node.

Funky185540 commented 2 years ago

Ah, I think I get it. The issue is that these images created by docker-compose have names which might just as well be names of real containers in a container registry. But the images may not exist in the registry yet, although they may become available at some later point in time, but they would likely be entirely different containers by then.

Is that correct?

Funky185540 commented 2 years ago

Would you mind creating a minimal compose file that reproduces this situation so I could test it on my PC? Just to make sure I actually work on what you're trying to tell me, before I end up fixing something that wasn't broken in the first place. That'd be really helpful for developing the feature.

raszi commented 2 years ago

Yes, but docker-compose is just an example in this case since you can create images with tags what exist in the Docker registry.

I believe the simplest what you can do is something like this:

$ echo 'FROM alpine' > Dockerfile
$ docker build --tag something .

This will create an image like:

$ docker images
REPOSITORY            TAG                 IMAGE ID            CREATED             SIZE
something             latest              c059bfaa849c        2 months ago        5.59MB
Funky185540 commented 2 years ago

This will create an image like:

Oh dear, it looks like podman and docker deviate in terms of behavior here...

❯ podman images
REPOSITORY                                 TAG         IMAGE ID      CREATED       SIZE
localhost/something                        latest      c059bfaa849c  2 months ago  5.87 MB
docker.io/library/alpine                   latest      c059bfaa849c  2 months ago  5.87 MB

I'll look into this, thank you!

raszi commented 2 years ago

But if podman can distinguish between these images then there should be a way!

Funky185540 commented 2 years ago

@raszi I updated the code, could you please perform the following steps again:

git clone https://github.com/Funky185540/topgrade.git
git checkout feature/upgrade-podman-containers
cargo run -- --only containers --verbose --dry-run

(Or you just git pull, whichever works for you) and tell me if it works better now?

raszi commented 2 years ago

There are two issues with the current version.

  1. It is still trying to pull locally tagged images and fails.
  2. It is trying to pull images where the tag is <none>:
 2022-02-11T14:23:59.546Z DEBUG topgrade::steps::containers > Pulling container 'memcached:<none>'      
 2022-02-11T14:23:59.546Z DEBUG topgrade::executor          > Running "/usr/local/bin/docker" "pull" "memcached:<none>"                                                                                         
invalid reference format                        
Funky185540 commented 2 years ago

Thanks for reporting!

  1. Yes, this is supposed to happen. But if the code works then you should see a DEBUG message stating "Skipping unknown container ''" and it shouldn't fail the whole topgrade step. Does that happen? Unfortunately I still don't know how docker could tell me whether a container was built locally...
  2. That is indeed an error, I haven't thought about that because I regularly prune my images. These containers should be properly skipped now.
raszi commented 2 years ago

No, unfortunately, it does fail:

 2022-02-11T16:58:14.696Z DEBUG topgrade::steps::containers > Pulling container 'docker-kcachegrind_kcachegrind:latest'
 2022-02-11T16:58:14.696Z DEBUG topgrade::executor          > Running "/usr/local/bin/docker" "pull" "docker-kcachegrind_kcachegrind:latest"
Error response from daemon: pull access denied for docker-kcachegrind_kcachegrind, repository does not exist or may require 'docker login': denied: requested access to the resource is denied
 2022-02-11T16:58:17.531Z ERROR topgrade::steps::containers > Pulling container 'docker-kcachegrind_kcachegrind:latest' failed: exit status: 1
 2022-02-11T16:58:21.536Z DEBUG topgrade::runner            > Step "Containers" failed: A step failed
 2022-02-11T16:58:21.536Z DEBUG topgrade::terminal          > Desktop notification: Containers failed

Retry? (y)es/(N)o/(s)hell
raszi commented 2 years ago

And if I press N, then:

―― 18:00:33 - Summary ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Containers: FAILED
 2022-02-11T17:00:33.681Z DEBUG topgrade::terminal          > Desktop notification: Topgrade finished with errors
Funky185540 commented 2 years ago

Sorry for the late reply...

Just to make sure: You did try this on commit 7d1294ba30f68d7b08dd7dd51280d223f58b048f of my branch? Because the error output from docker exactly contains the bit of string that should make it detect that this failed. If it works, it should print a WARNING in the debug output, which it didn't in your output above. :/

Funky185540 commented 2 years ago

@r-darwish Does the CI have some container runtime available that I could rely on for writing tests for docker/podman-related steps?

raszi commented 2 years ago

Yes, I was using the latest commit from your branch https://github.com/r-darwish/topgrade/commit/7d1294ba30f68d7b08dd7dd51280d223f58b048f

But now I deleted the whole directory and cloned it again to avoid any previous leftover artifacts, and the output is the same.

 2022-02-16T17:21:53.581Z DEBUG topgrade::steps::containers > Pulling container 'git-hours:latest'                                                                                                              
 2022-02-16T17:21:53.581Z DEBUG topgrade::executor          > Running "/usr/local/bin/docker" "pull" "git-hours:latest"                                                                                         
Error response from daemon: pull access denied for git-hours, repository does not exist or may require 'docker login': denied: requested access to the resource is denied                                       
 2022-02-16T17:21:56.108Z ERROR topgrade::steps::containers > Pulling container 'git-hours:latest' failed: exit status: 1                               

...

Retry? (y)es/(N)o/(s)hell

―― 18:22:36 - Summary ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Containers: FAILED
 2022-02-16T17:22:36.163Z DEBUG topgrade::terminal          > Desktop notification: Topgrade finished with errors
raszi commented 2 years ago

Isn't it the case that docker pull prints out this error to STDERR but you are expecting that on STDOUT?

Funky185540 commented 2 years ago

Isn't it the case that docker pull prints out this error to STDERR but you are expecting that on STDOUT?

That's a very good point, I wasn't thinking about that. I updated the code (did a force push) to also scan the output of stderr. When I find some time I'll hack up a set of tests to check for these errors so I don't always have to ask you to do it. Thank you for the valuable feedback so far!

raszi commented 2 years ago

Works like a charm!

 2022-02-18T11:27:15.724Z DEBUG topgrade::steps::containers > Pulling container 'git-hours:latest'
 2022-02-18T11:27:15.724Z DEBUG topgrade::executor          > Running "/usr/local/bin/docker" "pull" "git-hours:latest"
Error response from daemon: pull access denied for git-hours, repository does not exist or may require 'docker login': denied: requested access to the resource is denied
 2022-02-18T11:27:18.325Z ERROR topgrade::steps::containers > Pulling container 'git-hours:latest' failed: exit status: 1
 2022-02-18T11:27:20.860Z WARN  topgrade::steps::containers > Skipping unknown container 'git-hours:latest'
―― 12:27:48 - Summary ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Containers: OK
 2022-02-18T11:27:48.453Z DEBUG topgrade::terminal          > Desktop notification: Topgrade finished successfully
r-darwish commented 2 years ago

@r-darwish Does the CI have some container runtime available that I could rely on for writing tests for docker/podman-related steps?

I don't think so but Topgrade doesn't have a proper feature testing anyway. Is this ready to be merged?

Funky185540 commented 2 years ago

I don't think so but Topgrade doesn't have a proper feature testing anyway. Is this ready to be merged?

Unless you have any remarks regarding the code or the review I added to it yesterday, this is good to go from my POV. :)

r-darwish commented 2 years ago

I’ll do another review and merge. Thanks you so much for this effort

Funky185540 commented 2 years ago

I’ll do another review and merge. Thanks you so much for this effort

Sure thing, thank you for developing and maintaining topgrade!

r-darwish commented 2 years ago

Merged