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

Skip updating containers where no local image info can be retrieved #612

Merged
merged 3 commits into from
Aug 18, 2020

Conversation

piksel
Copy link
Member

@piksel piksel commented Aug 7, 2020

Partially reverts #571, since it causes dockerClient.StartContainer() to crash since it uses imageInfo to rebuild the config.

Fixes #584

@piksel piksel requested a review from tammert August 7, 2020 16:18
@piksel piksel marked this pull request as ready for review August 7, 2020 16:18
@piksel piksel requested a review from simskij as a code owner August 7, 2020 16:18
@tammert
Copy link
Member

tammert commented Aug 8, 2020

Won't this reintroduce #215 though? In the situation where the running container uses an image that it not available locally (anymore).

@simskij
Copy link
Member

simskij commented Aug 8, 2020

@tammert

Won't this reintroduce #215 though? In the situation where the running container uses an image that it not available locally (anymore).

It might very well do that, however - it will still grab oldImageID from ContainerJSONBase, which I think is the crucial part of your previous fix, right?

What happens right now, is that all other calls to ImageInfo throughout the code will try to dereference a null pointer, and as a result, crashes watchtower. For instance:

https://github.com/containrrr/watchtower/blob/master/pkg/container/container.go#L166-L208

I think any suggestion as to how to do this differently is welcome, though, as relying on ImageInfo seems to be a fragile solution in general.

I'm holding off on approving this review until we've sorted out this discussion. 👍

@piksel piksel force-pushed the revert-571-no-such-image-fix branch from 21d7777 to 626ef9d Compare August 9, 2020 21:01
@piksel piksel requested a review from tammert August 9, 2020 21:04
@piksel piksel force-pushed the revert-571-no-such-image-fix branch from 626ef9d to cc96db9 Compare August 9, 2020 21:07
This will allow watchtower to continue even though the image info for a
container cannot be retrieved. If this happens one warning will be emitted
and the container will be skipped, unless NoRestart or OnlyMonitor is supplied
Copy link
Member

@tammert tammert left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@piksel piksel changed the title Partially revert "Image of running container no longer needed locally" Skip updating containers where no local image info can be retrieved Aug 18, 2020
@simskij simskij merged commit 5efb249 into master Aug 18, 2020
@piksel piksel deleted the revert-571-no-such-image-fix branch May 5, 2022 12:23
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.

Portainer Disappeared After Update
3 participants