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

[core] Fix pidfile bug for 5.2.2 #1435

Merged
merged 1 commit into from
Mar 17, 2015
Merged

[core] Fix pidfile bug for 5.2.2 #1435

merged 1 commit into from
Mar 17, 2015

Conversation

degemer
Copy link
Member

@degemer degemer commented Mar 16, 2015

And thus add the ability to be restarted properly after a SIGKILL (since
the pid in the pidfile is now tested)

@degemer degemer changed the title [core] Fix pidfiel bug for 5.2.2 [core] Fix pidfile bug for 5.2.2 Mar 16, 2015
log.error(message % self.pidfile)
sys.stderr.write(message % self.pidfile)
sys.exit(1)
# Check if the pid in the pidfile corresponds to a running process
Copy link

Choose a reason for hiding this comment

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

let's use psutil.pid_exists, it's more concise, easier to read and works on Windows (even though this part of the code is not run on Windows) http://pythonhosted.org/psutil/#psutil.pid_exists

@degemer degemer force-pushed the quentin/5.2.2-pidfile-bugfix branch from fb3c0af to d433efa Compare March 16, 2015 18:31
And thus add the ability to be restarted properly after a SIGKILL (since
the pid in the pidfile is now tested)
@degemer degemer force-pushed the quentin/5.2.2-pidfile-bugfix branch from d433efa to 741de61 Compare March 16, 2015 18:35
@jkoppe
Copy link

jkoppe commented Mar 17, 2015

Now that datadog-agent runs under supervisord, is it necessary to write pid files like this?

@alq666
Copy link
Member

alq666 commented Mar 17, 2015

@remh is this for CentOS5 that we still carry pid files?

@remh
Copy link

remh commented Mar 17, 2015

This pull request is to fix a bug that was introduced with 5.2.

But you are right a lot of this logic is not needed anymore, we have a card in our board to clean this but it won't be done before 5.4.0.

@degemer
Copy link
Member Author

degemer commented Mar 17, 2015

Pidfiles for processes spawned by supervisor were introduced to fix #941 (so that old commands dd-agent, dogstatsd and dd-forwarder display a correct status).

@jkoppe
Copy link

jkoppe commented Mar 17, 2015

Gotcha. This looks like an adequate, simple improvement until the day comes when this code can be ripped out. Thanks!

remh pushed a commit that referenced this pull request Mar 17, 2015
@remh remh merged commit 836a3ca into 5.2.x Mar 17, 2015
@degemer degemer deleted the quentin/5.2.2-pidfile-bugfix branch March 20, 2015 20:01
degemer added a commit that referenced this pull request Apr 17, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 17, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 17, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 20, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 21, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 30, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 30, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request Apr 30, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request May 4, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request May 4, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request May 7, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request May 11, 2015
It's a legacy command which shouldn't be needed after #1435
degemer added a commit that referenced this pull request May 12, 2015
It's a legacy command which shouldn't be needed after #1435
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.

4 participants