-
Notifications
You must be signed in to change notification settings - Fork 179
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
Handle config changes on docker updates #137
base: master
Are you sure you want to change the base?
Conversation
Hi @mcuadros your app is super useful and I would like to integrate my changes so I can contribute to this amazing project. This change allows me to start / stop containers and let ofelia keep running, without restarting or possibly interrupting other jobs. It also supports hybrid configs (INI + Docker) or just one or the other. If there is any suggestion or comment, please let me know |
Thanks for your contribution. Also, don't be shy to ping me as a reminder if it takes too long :) |
Hi @trane9991, I have been using this for a while now and it seems to be working just fine. Re pinging as requested |
I am also very interested in this functionality getting merged! Thanks for this contribution! |
Sorry for the delays, will try to review it this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments.
The most critical issue is the removal of support job-local
and job-run
configurations with labels.
It is a breaking change, and some users (including me) using it, so I cannot accept this PR, until original configuration capabilities are preserved.
|
||
const HashmeTagName = "hash" | ||
|
||
func getHash(t reflect.Type, v reflect.Value, hash *string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using well-tested https://godoc.org/github.com/mitchellh/hashstructure lib for hashing, instead of implementing a custom one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That library does not support choosing the hashed elements but rather has an ignore list. I could switch but it would require ignoring instead of specifically adding the fields. I have no problem doing this if you prefer
@trane9991 changes are now ready |
Since this PR is very related, please let me share a question/thought in this discussion. There should never be more than one ofelia container reading job-exec configs from docker labels, in particular with live scanning. Otherwise each job-exec will be executed from multiple ofelia instances simultaneously. When using job-run, I am kind of forced to have additional ofelia containers, namely one for each project using job-run. (Otherwise part of each project's config would be with the central ofelia container, which I do not want.) In the many additional ofelia containers, I cannot configure job-run from labels, since then I would be scanning job-exec labels from each of them, which is not good as observed above. So, shouldn't job-run labels also be re-scanned from all containers, such that one docker daemon needs only one ofelia container? Thx. |
@bertbesser let me start by some definitions: job-exec: this job is executed inside of a running container. Docker labels are only available inside a running container, so I think the only job type that can be dynamic is job-exec (runs in the container that has the labels on). Job-run will create a new container, and as such, that container should have the labels attached that tell what container to execute, which makes the process recursive. In my opinion, if you want to make job-run dynamic, I would simply update the ofelia ini file and let that be auto reloaded by ofelia, the same way that it does with the labels. The idea is to make not only labels but also ini files be reloadable. I do not think its advisable to have more than one ofelia container, as it would execute jobs N times, particularly the labels based ones. As per docker docs, changing labels is not allowed after container creation, so you can't simply update the labels on the ofelia container either. |
@rdelcorro Thank you for taking the time to repond! I believe that recursion is not a problem. Assume that my service container, e.g. nextcloud, has ofelia labels for a related cron job, e.g. contacts export. Then scanning nextcloud's ofelia labels would allow to Regarding updates to the cron job definition. I would have to change nextcloud's labels and restart the nextcloud container. In particular, this way the cronjob config is stored in my nextcloud git repo. Instead, changing ofelia's ini file would require me to store nextcloud related configuration in my ofelia git repo. (Note: I know that I could work around the problem by either job-exec-ing in the nextcloud service container, or job-exec-ing in a container that is dedicated to running the export job. However, this does not work well with ofelia's logging, i.e. logs would not appear in the container running the export job. Instead, they would appear in ofelia's container. Using job-run, logs will appear in the correct container.) |
@bertbesser I had the exact same use case like yours but due to file permissions and volume mounts I decided to use job-exec instead and run inside the same container. Since what you requested was trivial to implement, I already added that feature. Feel free to test the pre built container or build your own: docker pull rdelcorro/ofelia:handleChangesOnDocker |
Wow. Thanks for the quick addition. I tested and noticed that there is a race condition where does the Before digging deeper into the issue, let's see what @trane9991 has to add. I can imagine that picking up Cheers :-) |
@bertbesser The ofelia container is polling docker for changes, so there is a chance were the source container (the one that has the labels) is either not fully started or it has terminated. I do not consider this a big deal as the “last run” will simply fail on a shutdown scenario. This is also a reason why I prefer job-exec for this. If the container is gone, the task is not even started. Running more than one ofelia container or process will lead to problems in any case, as those jobs will run N times. Remember that ofelia does not know that other ofelias are running (same applies if you use an ini file in multiple ofelia binaries). Docker labels are global to the docker daemon, so multiple ofelia containers will also read all the labels. You can make this work with multiple different ini files though. This change just lets you specify a job-run inside an external container but I do not consider this breaking anything, besides adding this support. In the past, job-runs on containers != than ofelia were ignored. This is adding that feature. In the past, you should have not added them to external containers, so this is not breaking anything. |
I build your image, @rdelcorro - great iniative, something which was missing in the main version! However, when I try to start with The README says to use it - but when I remove the flag, it seems to work as expected. Am I getting something wrong? |
@miggland the flag is now deprecated. I would have to update the readme. This should also be corrected: https://github.com/mcuadros/ofelia/pull/137/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R100 |
With the assistance of @githubsaturn, I was able to put together a working instance of Ofelia as a One Click App for the open source Caprover PaaS solution. However, without this pull request being accepted and a new docker image built, then all my effort has been wasted. After all was said and done, I ran into my last hurdle, with my research leading me to this pull request. I am disheartened to see a full 30 day wait on a pull request on a solution that I am certain solves an issue for others as well as myself. I cannot in good conscience promote a One Click App for Caprover until this has been resolved. To more immediately resolve this problem and to solve it for others in the long term, I have decided to hard fork the version of Ofelia as provided by @rdelcorro , (see https://github.com/rdelcorro/ofelia) as the license for Ofelia is currently MIT. I am in a unique position that I am self employed with my own software development company and am generally available 7 days a week, 365 days a year as I am on-call and always keep a 4G LTE/5G modem and my laptop on me at all times with a 4G LTE smart watch and phone which alerts me to opened issues, etc. This affords me the ability to pledge that I can do my best to maintain the project in a manor that benefits everyone. (as much as possible, anyway). The new project name will be Chronos. I will be publishing the new project at https://github.com/PremoWeb/Chronos. It will remain MIT licensed. Thank you for building a great piece of software. Let's improve upon it! |
And you think changing the license from MIT to GPLv3 is an improvement? Do you really expect people who redistribute your new project to open source all their code? |
Realistically, no. But for liability sakes, yes. It's a formality that transcends speculation. It provides legal recourse in the event something awful happens like a huge multinational corporation decides to use and improve the code but refuses to hand over a copy so everyone can benefit. Without it, there is nothing we can do. With it, we have at least some semblance of legal recourse. We as in, the community at large or it's contributors. |
I. for once won't be able to use your new project commercially and instead will be forced to continue using the MIT-licensed one. And thus I won't be able to contribute even if I want to. My company explicitly bans use of any GPL open source projects, unless we don't redistribute. I don't think my case in in any way unique. |
As a self-imposed general rule, virtually all projects I work on are GPLv3 licensed. In this particular case and after talking it over with one of my colleagues, I can see your side and fully agree with what you're saying. We'll go ahead and keep the MIT license for this project and move to benefit in another way, which is to encourage sponsorships. We'll accept donations for our work and where applicable, chip in to the contributors as funds are available. In this way, we can minimize any negative impact for the very unlikely scenario where some big company benefits and the contributors and users of the software don't. I'll make the adjustment in my previous post, leaving this one intact. In the spirit of free and open software, I feel this is the best move. |
Per topic brought up here, mcuadros/ofelia#137 (comment) and per conversation with a colleague, we're going to keep the original MIT license and instead, encourage donations and in turn, we'll distribute to contributors when and were we can.
Thank you, Nick! |
As the author of this PR I thank you for keeping this project alive. The owner does not seem to have time for the community and that’s ok. We just need to pass it over and keep it running. I am using this version for almost a year now and it works great. I will start contributing to your fork if the license remains unchanged as discussed above. Thanks |
You're welcome. I just began learning GoLang last year but only really started delving into it this year so I welcome contributions from people more skilled than me in this language. I'm really enjoy Go and looking forward to using it in many more projects in the future. I also very much appreciate that the original owner has put so much into making a solid alternative to cron for use in Docker Swarm. I aim to have this operational tonight. I have the first docker image pushed to github's container registry. You can follow the project at https://github.com/PremoWeb/Chadburn. The docker image is at https://github.com/PremoWeb/Chronos/pkgs/container/chadburn. I plan to be auto-building and pushing the image to Github's container registry because Docker Hub imposes unreasonable limits and I am using a Paid Github plan. Edit: The name was changed from Chonos to Chadburn to avoid conflict with another project. |
Good to hear. As a piece of advice, would you consider pushing releases to Dockerhub? No need to push nightlies, but once in a while, when you feel that you've achieved a milestone, a new release (and a celebration) is justified. |
Yes. I just need to get the Github action workflows in order for this to work and have it only trigger when I set a new release. |
I have tagged releases triggering pushes to both Docker Hub and Github Container Reigstry. |
Thanks much for your decision an bias for action. Would be possible to provide also arm/v7 images from the registry?, I was carefully following this project to be able to use a 'out-of-the-shelf' solution and your fork solves all my pending requirements ;) |
Firstly, you're welcome. Glad I can be of help to those seeking an off the shelf solution. That was my position too, prior to having my decision to hard fork this project. To answer your question, I am not familiar with the arm/v7 architecture but I'm certain we can get that implemented. Hopefully quickly. Let's take this issue elsewhere. I've opened a new issue on the new Chadburn project, referencing you and this comment to track it and give you an easier way to find it later from the Github website.
Glad to be of help! Let's take this issue to the new project called Chadburn on it's official Github repository. Follow this link to see my response and I've got a question for you there. PremoWeb/chadburn#2 If we can get ARM/v7 support for Chadburn, we'll work to do a pull request to get the same support in Ofelia. -Nick |
Hello there! I'm very happy about such active interest and community, at the same time I really sorry that I don't maintain this "properly", however, there are reasons for that:
I would be happy to see Ofelia evolve and these changes merged, at the same time I know that I don't have enough time and resources to test everything properly and make sure we don't break things with major changes for many people who use Ofelia (it is also likely that many people don't use versioned docker images, because they were introduced not so long ago). I would be happy to see more people added to the contributors list who actually uses the tool daily and can provide better support than I do at the moment. Part of the problem, that I'm not the original creator, I am just a guy who created a fork to add support of docker labels, and then @mcuadros gave me write permissions so I could merge my changes. @mcuadros if you are around, maybe you can create ofelia organization and give more people permissions to manage the repo? @maietta is a great candidate :) I would add these changes in a new branch, let the people use it for some time and then release v1.0.0 once we think it is ready. |
Thank you for the update and encouragement. Collaborative software development can definitely be time consuming. I am lucky enough to have enjoyed more than 20 years working for myself as a software developer. This afforded me the flexibility to study and adopt new technologies, techniques and best practices over the years. I'm currently studying and using the Go programming language in a few projects so I was glad to see Ofelia having been written in that language. This also means more flexibility to get it implemented in more obscure CPU architectures. When I performed a hard fork of Ofelia called Chadburn, I decided to carve out a week or so to begin looking over pull requests for Ofelia and to see about getting those implemented into Chadburn. As I implement each of them, I am making sure to link back to the original pull requests for Ofelia and also when performing commits, doing the same but also tagging the author. In this way, Ofelia's project pages are dynamically linked to show the relevant activity. At some point soon, I should have most or all of the current pull requests implemented in Chadburn. This could in effect, become Ofelia's new testing branch, saving you time (since I'm already working on it). If at some point both Ofelia and Chadburn are generally the same software again, then I can retire Chadburn since Ofelia has been around much longer and already has the community behind it. We'll see what @mcuadros wishes to do but just know that I don't mind maintaining a separate project to speed things up. |
Thanks @mcuadros for replying. I do work on golang for a living so I know a thing or two. I am willing to be part of the official collaborators if possible and help with the project. There was never a complaint regarding "proper" management. We do this on free / spare time and we love what we do so much that we enjoy doing so. Let's do this together as a community and keep Ofelia going |
@taraspos Hello, any chances this PR will be upstream? For now, the original Ofelia has gone forward from the point when @rdelcorro did the fork. So, I'm stuck which of them to use in my projects, Seems that it's better to merge such a change to make Ofelia an obvious choice among others. Probably I am not the one with that kind of thoughts.. |
The purpose of this PR is: