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

Do not kill services if image download fails #34

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

andresmoschini
Copy link
Contributor

@andresmoschini andresmoschini commented Apr 16, 2020

Hi @djmaze!

You have done an amazing job with this tool. Thanks!

I had the same problem that @rmie, so I have applied the change that he suggest in #28, then I realized that it was breaking shepherd script execution, so I added an exit code validation.

Could you review my changes? It would be great having this fixed in the official image.

shepherd Outdated
@@ -23,18 +23,22 @@ update_services() {
image_with_digest="$(docker service inspect "$name" -f '{{.Spec.TaskTemplate.ContainerSpec.Image}}')"
image=$(echo "$image_with_digest" | cut -d@ -f1)
echo "Trying to update service $name with image $image"
docker service update "$name" $detach_option $registry_auth --image="$image" > /dev/null
docker pull $image && docker service update "$name" $detach_option $registry_auth --image="$image" > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, this means all images are pulled to the machine that shepherd is running on. Which means quite some bandwidth and storage overhead.

As docker manifest is now available, I am really curious to try this out instead: https://stackoverflow.com/a/56316948

Would you like to adjust your PR accordingly?

Copy link
Contributor Author

@andresmoschini andresmoschini Apr 16, 2020

Choose a reason for hiding this comment

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

Thanks for your quick response.

I going to read your link and will try to adjust my PR.

In the meantime, I have a question (maybe naive, sorry). Will not the images be pulled to the nodes anyway? I mean, the nodes need it to run, what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will not the images be pulled to the nodes anyway? I mean, the nodes need it to run, what am I missing?

Each image will only be pulled to the node the corresponding service runs on. With your pull approach, all images end on the node which runs the shepherd service as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I understand now, thanks!

@djmaze
Copy link
Collaborator

djmaze commented Apr 19, 2020

Thanks!

@djmaze djmaze merged commit 426ca6a into containrrr:master Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants