Add daemonize option to 'docker-sync start', closes #51 #65
Conversation
|
will take me a while to review this. You would do me a great favour in adding inline comments for the semantic ideas behind "what is done why" |
|
@EugenMayer Sure, would you like me to add Comments to the PR aka on GitHub or rather in code itself? |
|
inline in the code would be awesome! |
|
@EugenMayer Added some inline comments on new/changed things, you can check last comment in order to make it more readable |
|
Thank you a lot, could take some days for me to actually approve / think over / test it, but in general, i am tensed to see what you build there! |
|
@midN could you merge in the current master so we can start working on this issue? |
Conflicts: Gemfile tasks/sync/sync.thor
|
@EugenMayer Updated |
|
Well its scheduled for the next release - but as always, its done when its done. Feel free to create your own branch and merge this into the master, and report back how things work out. But i do not expect this to be trivial. |
|
Please add it to stack as well |
|
Yes, if it is trivial to add -d to docker-sync-stack, that would be great. If not, this PR feature is awesome anyway! |
Conflicts: Gemfile Gemfile.lock docker-sync.gemspec tasks/sync/sync.thor
|
Thank you for still working on this @midN - i am still very interested in merging this in - i currently try to stabilize the core functions so we have stable sync images and strategies, then moving on with this - daemonize can hide a lot of issues or people will at least not pay attention, thats why delaying it can make a bit of sense right now! |
|
|
||
| class Sync < Thor | ||
|
|
||
| class_option :config, :aliases => '-c',:default => nil, :type => :string, :desc => 'Path of the docker_sync config' | ||
| class_option :sync_name, :aliases => '-n',:type => :string, :desc => 'If given, only this sync configuration will be references/started/synced' | ||
|
|
||
| desc 'start', 'Start all sync configurations in this project' | ||
| method_option :daemon, :aliases => '-d', :default => false, :type => :boolean, :desc => 'Run in the background' | ||
| method_option :app_name, :aliases => '--name', :default => 'dsync', :type => :string, :desc => 'App name used in PID and OUTPUT file name for Daemon' |
There was a problem hiding this comment.
Apologies in advance if this isn't a reasonable request - I'm not as familiar with daemonizing ...
One thing I've noticed while using this branch in comparison to using master is that you have to specify names if you're working on more than one project at the same time (such as a micro-service stack). In the original approach it wouldn't matter, but with this approach you cannot simply replace docker-sync start with docker-sync start -d if you plan on utilizing more than one project.
Is it possible to utilize the name of the first defined configuration as the default name for the PID, rather than resorting to just dsync?
|
When you daemonize, you have to be able to manage that process, restart and maintain it. Also you have to cope with the hidden logs, which might give you an idea why things go wrong. This means, there is quiet some work in the docs, in the way issues have to be reported and overall in how the application has to be managed. A lot of documentation have to be adjusted. Moreover, the patch is also not too simple and needs quiet some testing. That said, i was focusing on getting docker-sync robust, coping with all the issues, edge cases, smaller adjustments and glitches we might have. All the things you see better, when its in the foreground. If anybody volunteers, feel free to test this branch, suppose changes to the upper docs on the procedure. I put it simple - help is really still needed here, anybody want to add awesomeness here? :) |
|
What would be your preferred approach for helping with this PR? I could make PR requests against midN's master branch, which would then show up in this PR if they're accepted/merged. But in terms of updating documentation, is there a good approach to submitting updates for things that haven't yet been merged? |
|
@masterful thank you for joining on this! i would say it depends on the way @midN would like to have it. We could otherwise merge this one into a branch here in the docker-sync repo and you contribute against that - from my thinking, that makes most sense: it makes contribution possible for all, all PRs are visible, people see were the work is going and what is still opened. For this pull request, i need at least someone doing a deeper integration test, that means:
Then we should plan, ob this fundament, how we should be planing to have the daemonize mode working: Those topics can be dicussed without doing any contribution - that planned, we can move on merging this one, creating the tickets for the integration phase and work on the next issues until we have this one ready for 0.2.0 -- docs are mainly in the wiki, so that will be rather the hard part (not too easy to prepare ). I guess we should create a new page wich includes all the new content - later if needed, we distribute ( move ) it into the pages we already have, if needed. -- How does that plan sound? |
|
Sounds like a good plan! Addressing your concerns/questions:
The idea of merging this into a separate branch on this repo appeals to me - as you say, I think it would be a good idea to keep collaboration as close to the original repo as possible.
I think I'm comfortable with performing some of that in depth testing using the boilerplate. I'm obviously already testing this code with my own configurations (using unison) - I can try a few tests with the rsync approach as well.
I don't know how easy it would be to 'pick up the process again' - I'm not certain that there'd be a good way to go about that when we start it in daemonized mode. I'll have to take a closer look at how the gem behaves to see if it's possible in a sane manner. You can obviously
I don't believe it should be, no - at least not until it behaves equivalently to the current mode. It would also make discovering issues with initial setup more difficult (as you've mentioned).
I'm guessing this is regarding having multiple configurations within the same
I was wondering about this as well. As it's currently configured we're outputting the logs to the same folder as the PID file... I think the current configuration (defaults to
I'd have to look closer at how the |
That means, if you start different docker-sync.yml based projects. Supporting several sync-endpoints in one docker-sync.yml is easy We are basically on the same page. So i will merge this patch into a branch so you can start working against that one |
|
This PR looks very promising. Is there any way to daemonize only when all sync endpoints have been started? In commands such as |
|
@michaelbaudino that's a fantastic suggestion - but perhaps not an easy one to implement ... I'll try to take a look at that when I get a moment |
|
If it can be useful for other people stumbling upon this PR, here is the (fragile and ugly) script I use to start Feel free to give any constructive feedback in the gist comments ❤️ In a nutshell:
PS: the same thing is probably achievable using |
|
@michaelbaudino I've updated my fork which has a PR against @midN 's branch: https://github.com/midN/docker-sync/pull/1 If you're comfortable testing it ahead of time, you can download my fork here: And then unzip it and use: gem build docker-sync.gemspec
gem install ./docker-sync-0.1.5.gemThis will install the gem from the local source (my version). The usual caveats of multiple installations if you're using This PR also has the added benefit of a slightly faster |
|
very interesting @masterful thank you for your work. How long are you using this for now? Does it cope with multiple sync points? How do you end a sync ? |
|
I guess I've been using it for about a month and a half. I haven't tested much beyond With my rewrite it copes fairly well with multiple sync points. You can end a sync by calling |
|
Interesting, so i think thats a fair limitation for now. |
|
There's no "attaching" per se... But you can always `tail -f` them...
…On Wed, Feb 1, 2017, 5:11 PM Eugen Mayer ***@***.***> wrote:
Interesting, so i think thats a fair limitation for now.
How would i get attached to the sync logs if i would need to, any strategy
yet for this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#65 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUMukv1apaQ2Vc3WzlKvgz_i2YCt0KVks5rYQMMgaJpZM4JT0w9>
.
|
|
well, were would the user expect the logs, is there a specific location ? |
|
Ah - by default the logs are configured to go to So, for example, suppose your docker-sync was something like: syncs:
appname-sync:
...
another-sync:
...Then you can expect |
|
Hey everyone, great work so far. I've created quite a few daemonized processes myself and one of the most common things to consider is something i see you have discussed here already, the ability to interact with the process once it has been daemonized. It is not the easiest to do this cross platform, and it is even less possible to achieve when you use code to do the daemonize for you, like the daemonize package imported to this PR, rather than using the proper tool (systemd on Linux, launchd on macOS, windows I'm unsure). If anyone is not familiar with how daemons work to better understand what i mean above, some suggested reading would be;
Although i am unable to have any free time to contribute code, I hope this helps someone get started in the right direction. A final word. It might be "ok" to just daemonize as you have in this PR, release it as-is, but put a warning in that explains it is experimental and you will not be able to stop or clean docker-sync. If you wish to kill the daemon you should be able to locate the pid and kill it yourself manually, or not use the daemonize option at all. Just a thought |
|
@chrisdlangton thank you for the hints and feedback!. I am perfectly aware of lanuchd and custom services, since we are already using it a lot with boxen. Eventhough we do not cross platform stuff, so a OSX solution will do it, i do not really like to have launchd in place. It involves root access at least during installation and also i do not really like the way of configuration. Often it is a 1:1 relation to the service, so start "sync", but docker-sync could be started several times, per project - those kinda things become very complicated with launchd or at least unhandy - same goes for upstart/systemd. Sure we could place a system-wide "project" list to start, but people want to start X this time, Y the next time. A system level integration does not make sense here, at least in this scenario - a lot of other things do work out very well with launchd. Thus said, i would rather stick to a local "daemon" solution, putting its pid into projectX/.docker-sync/pid , its logs into projectX/.docker-sync/logs while we start it using .../projectX as CWD. |
|
Thanks for looking at this @chrisdlangton - I appreciate someone who works more with daemons chiming in. I believe the gem that we're bringing in to do the daemonization of this script does use the double forking trick, while remapping stdout and stderr to the output file we specify. As @EugenMayer points out, having a system-wide managed docker-sync may end up being less helpful since they're tied pretty closely to the project(s) one happens to be working on at that time. You can absolutely continue to @EugenMayer - I spoke with @midN about this on my PR, and he would prefer that I submit my PR against this repo - which would mean closing this one and opening a new one from my fork. I'll do so today, but in the mean time I'd like to clarify: What is the rationale for maintaining the running list? If it's just to see what sync's are currently running, would |
|
@masterful thank you. go on with the PR, thats a good idea. I think it is a valid requirement to update the .gitignore file and nothing really scary knowdays, be it vendor or .idea ans so on - for such a tool it just make sense The point behind the folder is the context. Using this, we can still do a simple docker-sync stop, and basically guessing from the CWD what daemon shall be controlled. Also for the people, its much easiert to check the logs in myproject/.docker-sync/log instead of "somewhere/log/.log In general, i really appreciate the amount of thoughts going into this and i also think, that this feature, combined with some other things, will lift docker-sync to a new level with 0.2.0 0 |
|
closing due to #229 |
Added 'daemonize' option currently only to
docker-sync start, as i see no reason to add it todocker-sync-stackas it runs whole thing in single foreground anyway.Also skipped
docker-start syncdaemonization as well, since it runs only sync and no daemon would be required i believe.I think this servers enough purposes of starting
docker-sync startin the background with proper configurable attributes.I can now use it to start sync whenever my Mac boots and keep developing flawlessly with combination of
terminal-notifier, thank you for the awesome app @EugenMayer 🎉 .