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

Start rsync daemon in the foreground to prevent zombie processes #9939

Merged
merged 1 commit into from
Jul 20, 2016

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Jul 19, 2016

For the rsync-daemon strategy starts the rsync daemon in the foreground, checks that it has been started by verifying that a pid file exists for it, and then proceeds with the copy.

The foreground exec is later terminated when the daemon process is killed.

Fixes #9882

@csrwng
Copy link
Contributor Author

csrwng commented Jul 19, 2016

@bparees ptal

if err == nil {
break
}
if s.daemonErr != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to wrap a mutex around this to ensure you're getting the current value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and should this check come before running the checkScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the order matters since we are looping. Also, not sure that a mutex is needed ... if we don't catch it immediately, we'll just catch it the next time we come around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ordering was a minor optimization.

as for the mutex, i'm wondering if you're going to get race warnings from docker. or the general non-guaranteed outcome of reading shared variables w/o a memory barrier.

@csrwng
Copy link
Contributor Author

csrwng commented Jul 19, 2016

Updated to use a channel for the error

@bparees
Copy link
Contributor

bparees commented Jul 19, 2016

lgtm [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 19, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6566/) (Image: devenv-rhel7_4632)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 39e65ab

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@csrwng
Copy link
Contributor Author

csrwng commented Jul 20, 2016

Flake #8571

@csrwng
Copy link
Contributor Author

csrwng commented Jul 20, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 39e65ab

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6566/)

@openshift-bot openshift-bot merged commit 4bdf42a into openshift:master Jul 20, 2016
@csrwng csrwng deleted the rsync-zombie branch December 21, 2016 20:59
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.

3 participants