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

feat: ignore removal error due to non-existing containers #1481

Merged
merged 2 commits into from
Dec 6, 2022
Merged

feat: ignore removal error due to non-existing containers #1481

merged 2 commits into from
Dec 6, 2022

Conversation

nothub
Copy link
Contributor

@nothub nothub commented Nov 20, 2022

This PR allows watchtower to ignore a failure to remove containers that doesn't exist. This can be useful when the start of the containers is managed by docker compose or an external system such as systemd.

Watchtower causes an issue for me in combination with the --abort-on-container-exit flag for dockers compose up subcommand.

Docker compose removed the container on its own after the container was stopped. This leads to an error when watchtower attempts to remove the container.

Error: No such container: 425173...

The error is thrown by this ContainerRemove call.

Fixes #1480

@nothub nothub requested a review from simskij as a code owner November 20, 2022 23:59
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congratulations on opening your first pull request! We'll get back to you as soon as possible. In the meantime, please make sure you've updated the documentation to reflect your changes and have added test automation as needed. Thanks! 🙏🏼

@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 64.76% // Head: 66.19% // Increases project coverage by +1.42% 🎉

Coverage data is based on head (0c9030d) compared to base (a4d00bf).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
+ Coverage   64.76%   66.19%   +1.42%     
==========================================
  Files          24       24              
  Lines        2310     2313       +3     
==========================================
+ Hits         1496     1531      +35     
+ Misses        718      682      -36     
- Partials       96      100       +4     
Impacted Files Coverage Δ
pkg/container/client.go 41.29% <100.00%> (+9.45%) ⬆️
pkg/container/container.go 51.76% <0.00%> (+1.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@piksel
Copy link
Member

piksel commented Nov 22, 2022

Just skipping all container removal sounds a bit inflexible, and only fixes the problem if all containers are set to auto-remove.
It's probably better to ignore the error and try to create the new container anyway.
Perhaps we can even detect that the container is set to auto-remove (should be the same as docker run --rm.

@nothub
Copy link
Contributor Author

nothub commented Nov 22, 2022

Yes you are right, checking for each container is a better choice.
I pushed a change with that behaviour.
The error is now skipped if it is of type ErrNotFound.

@nothub nothub changed the title Flag for skipping container removal Skip removal of non-existing containers Nov 22, 2022
@nothub
Copy link
Contributor Author

nothub commented Dec 1, 2022

Is there anything else you want me to change for the PR to be accepted?

@piksel
Copy link
Member

piksel commented Dec 1, 2022

I mostly haven't had time. Sorry. I will get to reviewing it as soon as I get some spare time.

@piksel
Copy link
Member

piksel commented Dec 6, 2022

Yeah, watchtower already checks for AutoRemove, but since this is just the docker-compose process manually stopping and removing the containers it's a special case. Ignoring that the container was not found when trying to remove it should be fine in general though.

nothub and others added 2 commits December 6, 2022 17:13
…ontainers that were removed by other means (errors of type ErrNotFound).

This can be useful when the start of the containers is managed by docker compose or an external system such as systemd.
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

This change seems to be the most sane course to take regardless of whether it's caused by the issue this PR attempts to fix.
I added a test for the "normal" case (container still existing after being stopped), as well as one for when the container could not be found.

@piksel piksel changed the title Skip removal of non-existing containers feat: ignore removal error due to non-existing containers Dec 6, 2022
@piksel piksel merged commit 3190ce2 into containrrr:main Dec 6, 2022
@nothub
Copy link
Contributor Author

nothub commented Dec 7, 2022

Awesome :) Thanks a lot for your work on this project!

@piksel
Copy link
Member

piksel commented Nov 13, 2023

@all-contributors add @nothub for code

Copy link
Contributor

@piksel

I've put up a pull request to add @nothub! 🎉

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.

Compatibility issue with docker compose abort-on-container-exit flag
2 participants