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

Handle config changes on docker updates #137

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rdelcorro
Copy link

@rdelcorro rdelcorro commented Dec 21, 2020

The purpose of this PR is:

  • Update tasks if dockers are started, stopped, restarted, or changed
  • Do not require a dummy task on the ofelia container just to use ofelia
  • Support INI and docker labels at the same time. The configs will simply be merged
  • Do not require ofelia to restart in order to pick up new or remove tasks

@rdelcorro
Copy link
Author

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

@taraspos taraspos self-requested a review December 21, 2020 08:30
@taraspos
Copy link
Collaborator

Thanks for your contribution.
This is quite a lot of changes. I will try to review it in the coming days.

Also, don't be shy to ping me as a reminder if it takes too long :)

@rdelcorro
Copy link
Author

Hi @trane9991, I have been using this for a while now and it seems to be working just fine. Re pinging as requested

@bertbesser
Copy link

bertbesser commented Jan 15, 2021

I am also very interested in this functionality getting merged! Thanks for this contribution!

@taraspos
Copy link
Collaborator

Sorry for the delays, will try to review it this weekend.

Copy link
Collaborator

@taraspos taraspos left a 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) {
Copy link
Collaborator

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?

Copy link
Author

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

README.md Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@rdelcorro
Copy link
Author

@trane9991 changes are now ready

@bertbesser
Copy link

bertbesser commented Feb 13, 2021

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.

@rdelcorro
Copy link
Author

@bertbesser let me start by some definitions:

job-exec: this job is executed inside of a running container.
job-run: runs a command inside of a new container, using a specific image.

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.

@bertbesser
Copy link

@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 job-run a new container for the export, and that new container does not need to carry any labels.

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.
However, I would like to decentralize cron config, i.e. store it with nextcloud instead of ofelia. Hence the question if ofelia can be made to pick up job-run configs from other containers' labels :-D

(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.)

@rdelcorro
Copy link
Author

@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

@bertbesser
Copy link

@rdelcorro

Wow. Thanks for the quick addition. I tested and noticed that there is a race condition where does the job-run after the container defining the related labels has already stopped (this could potentially lead to errors when, say, the nextcloud service to export contacts from was already removed).

Before digging deeper into the issue, let's see what @trane9991 has to add. I can imagine that picking up job-run labels from all containers is a breaking change. Why? Without the change each ofelia container will only respect job-run labels of itself. There might be users who run one ofelia container per project, where each ofelia container scans its own job-run docker labels. (This is easy to configure with e.g. docker-compose compared with building custom images including an ini file.) Picking up job-run labels from all containers will make those users' jobs run multiple times simultaneously, which is not intended.

Cheers :-)

@rdelcorro
Copy link
Author

rdelcorro commented Feb 14, 2021

@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.

@miggland
Copy link

I build your image, @rdelcorro - great iniative, something which was missing in the main version! However, when I try to start with command: daemon --docker I get the error mesage unknown flag docker'`

The README says to use it - but when I remove the flag, it seems to work as expected. Am I getting something wrong?

@rdelcorro
Copy link
Author

rdelcorro commented Feb 26, 2021

@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
I will change those later today

@maietta
Copy link

maietta commented Oct 1, 2021

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!

@teosoft123
Copy link

The project will be licensed under GPLv3, so that people are encouraged to contribute changes that could benefit everyone.

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?

@maietta
Copy link

maietta commented Oct 1, 2021

The project will be licensed under GPLv3, so that people are encouraged to contribute changes that could benefit everyone.

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.

@teosoft123
Copy link

The project will be licensed under GPLv3, so that people are encouraged to contribute changes that could benefit everyone.

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.
Perhaps you will reconsider the license change.

@maietta
Copy link

maietta commented Oct 1, 2021

The project will be licensed under GPLv3, so that people are encouraged to contribute changes that could benefit everyone.

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. Perhaps you will reconsider the license change.

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.

maietta added a commit to PremoWeb/chadburn that referenced this pull request Oct 1, 2021
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.
@teosoft123
Copy link

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.

Thank you, Nick!

@rdelcorro
Copy link
Author

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

@maietta
Copy link

maietta commented Oct 2, 2021

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.

@teosoft123
Copy link

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.

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.
People are more familiar with pulling from Dockerhub, largely because it's a default, no need to specify an alternative registry.

@maietta
Copy link

maietta commented Oct 2, 2021

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.

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. People are more familiar with pulling from Dockerhub, largely because it's a default, no need to specify an alternative registry.

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.

@maietta
Copy link

maietta commented Oct 2, 2021

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.

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. People are more familiar with pulling from Dockerhub, largely because it's a default, no need to specify an alternative registry.

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.

@maietta maietta mentioned this pull request Oct 2, 2021
@markfqs
Copy link

markfqs commented Oct 5, 2021

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.

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 ;)

@maietta
Copy link

maietta commented Oct 5, 2021

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.

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.

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.

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 ;)

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

@taraspos
Copy link
Collaborator

taraspos commented Oct 6, 2021

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'm not using the tool in my daily work anymore
  • I moved to a different country, new job and all the things that follows.

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.

@maietta
Copy link

maietta commented Oct 6, 2021

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'm not using the tool in my daily work anymore
  • I moved to a different country, new job and all the things that follows.

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.

@rdelcorro
Copy link
Author

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

TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 30, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 30, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 30, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 30, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 30, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 31, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 31, 2022
TheDevMinerTV pushed a commit to netresearch/ofelia that referenced this pull request May 31, 2022
@faerics
Copy link

faerics commented Jun 26, 2024

@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..

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.