-
Notifications
You must be signed in to change notification settings - Fork 316
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
keeping image latest always restarts service #526
Comments
@duritong this reads as though the notify to the next resource is still being sent even though the I don't recall this happening in puppet 5, has something changed? If this was the case every puppet for our local dev-envs run would have restarted every container on each apply, which is definitely not what used to happen. |
I recall something like that in the changelog for Puppet 6, but maybe I am remembering that wrong. But yes, this is basically what is happening here. |
The behaviour of the module is the same with Puppet 5. Same issue was reported five months ago: #479 |
I encounter same issue
|
I confirm this behavior on puppet 5.5.10. |
I suspect the underlying issue is that having the image resource trigger the service restart on update is probably too inflexible and instead there should be a specific resource that is part of the image class that should be notified and instead of:
Use something like:
Ideally it would be nicer if there was a simple notify resource put in place that each of the execs would notify and that was documented as the resource to use to notify I went and looked at our manifest, and as we were hooking of a similar Exec to the above (we hadn't managed to roll forward to pick up the changes landed) that's why we weren't getting impacted by this, because we worked out that needing to let some stuff update within the Not sure what would be the nicest way to integrate this with the existing codebase |
One of our Developers has come across this exact issue. It's a long piece of string... #468, #316, #297... 😄 If I read all of that correctly, the root of the problem is Docker doesn't provide an easy way to check the SHA of an image on the registry so you can compare it to local images. We found this: https://stackoverflow.com/a/46733555, which is not pretty, prone to break in the future, plus opens up a whole bunch of other questions (How do you get an Auth token?). It might just solve this once and for all though... If we were to go back to an The work is non-trivial, but I'm willing to give it a go. Thoughts, @electrofelix and @duritong? |
@lukebigum been a while since I thought about this. I suspect it would introduce issues dealing with auth so just moving the problem elsewhere. Unless there are some ruby API bindings for docker that make it possible to hook into the standard creds and get a response without retrieving the image. I did see a few about, but not sure if they are sufficient to use here. There is definitely a python api that is supported by docker, but I'm not sure it's a good idea to introduce that as a dep. Maybe one other way would be if a script checking for an update reset the tag back to the previous sha256 on exit as part of a trap? So the image doesn't get updated? Other than that maybe a script to talk to the registry is one way, just expect it'll have to be able to handle auth. Lots of ifs, buts, and maybe there. Sorry I don't see the right path so maybe your suggestion will take care of enough, and some script examples and docs for helping edge cases is the way to go |
I just stumbled upon |
This issue has been marked as stale because it has been open for a while and has had no recent activity. If this issue is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days. |
Related to #627 |
Would using a custom tag to denote the last pulled image work here for all cases? Basically retrieve the image and if tag reference is the same as the last image tagged with a last pulled tag, there is no need to notify the service to restart. Would need an additional parameter to allow control of the tag used for this. Presumably though it'll work for all docker clients instead of needing an experimental feature. |
Hello! 👋 This issue has been open for a while and has had no recent activity. We've labelled it with If you are waiting on a response from us we will try and address your comments on a future Community Day. Alternatively, if it is no longer relevant to you please close the issue with a comment. |
I believe it's likely going to still be impacting others however I'm no longer using puppet so I'm no longer directly impacted. |
This issue is still relevant. |
1 similar comment
This issue is still relevant. |
We managed to fix this, at least for our use cases. See #627 (comment) |
…atest" If image version is set to "latest" - there are changes on each run, even if nothing is really done - docker containers are restarted every time This is fixed here. However, this needs a change in using docker::run, which now needs an additional subscribe => Docker::Image[...]. See puppetlabs#627
…atest" If image version is set to "latest" - there are changes on each run, even if nothing is really done - docker containers are restarted every time This is fixed here. However, this needs a change in using docker::run, which now needs an additional subscribe => Docker::Image[...]. See puppetlabs#627
Describe the Bug
#468 tried to fix an issue with noop but now introduced that the exec is always triggered if using latest.
Expected Behavior
Exec does not propagate a refresh if nothing updates, and nothing gets changed during a run:
Service gets notified by image - idea is that service should get restarted when image changes.
BUT service even gets restarted when image is not changed (Status: Image is up to date for registry.code.immerda.ch/immerda/container-images/gitlab:latest), because the notify resource still triggers as a change. Which has also the sideeffect that the node always counts as changed and never has 0 changes.
This is on latest master using puppet 6.9.0
CC @electrofelix
The text was updated successfully, but these errors were encountered: