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

Na wait for rsync daemon #604

Merged
merged 3 commits into from
Sep 7, 2018

Conversation

nalbury-handy
Copy link
Contributor

During start up, our team has been getting:

rsync error: error in rsync protocol data stream (code 12) at /BuildRoot/Library/Caches/com.apple.xbs/Sources/rsync/rsync-52/rsync/io.c(453) [receiver=2.6.9]

In our testing we found that it took approx 2-4s for the sync container to not only be running, but for the rsync daemon to be listening on the desired port. The start_container method in rsync.rb seemed to handle most of these start delays with a sleep 3 but if the rsync daemon is still not available after 3s, the initial sync is attempted anyway, resulting in the above error.

The connection to the rsync daemon can be tested with the --dry-run flag. So I've attempted a health_check method that will use this call to wait until rsync is listening on the desired port and container IP.

This health_check method will wait until the rsync --dry-run... command returns a non empty string. It will only try 10times to prevent an infinite loop, and any issues syncing after that are handled normally by the sync method.

Thanks!

@EugenMayer
Copy link
Owner

that's a pretty good implementation and finding. let me review it roughly and merge it later on. very valuable, thanks!

@EugenMayer
Copy link
Owner

i think one of the issue here might be, that AFAIR sync_host_ip is nowdays optional : https://github.com/EugenMayer/docker-sync-boilerplate/blob/master/rsync/docker-sync.yml#L8

i see that we allow auto there but i cannot find where we potentially expand it. Did you try it with auto?

@nalbury-handy
Copy link
Contributor Author

nalbury-handy commented Sep 7, 2018

Yea in our config we don't explicitly set sync_host_ip and it defaulted to 127.0.0.1, which worked as expected.

@EugenMayer
Copy link
Owner

great thanks! Good to merge?

@nalbury-handy
Copy link
Contributor Author

Yup, thanks!!!

@EugenMayer
Copy link
Owner

I have to thank! Thanks for the contribution.

@EugenMayer EugenMayer merged commit 7281e3f into EugenMayer:master Sep 7, 2018
@EugenMayer EugenMayer added this to the 0.5.8 milestone Sep 30, 2018
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.

2 participants