Skip to content

/etc/init.d/go-agent stop stops the agent using pid from PID_FILE#5897

Merged
marques-work merged 3 commits intogocd:masterfrom
ibnc:agent_linux_issue
Feb 26, 2019
Merged

/etc/init.d/go-agent stop stops the agent using pid from PID_FILE#5897
marques-work merged 3 commits intogocd:masterfrom
ibnc:agent_linux_issue

Conversation

@ibnc
Copy link
Contributor

@ibnc ibnc commented Feb 22, 2019

see #5858 for details on the issue this addresses

I would have ideally used pkill -u go -F $PID_FILE but pkill on centos 6 does not have a supported way to use the pid file to stop a process. This approach should ideally work on all of the linux distros we support.

I'm also unsure why we were passing arguments we weren't using to killproc This line is the one I'm referring to here


if [ $? -eq 0 ]; then
killproc -p $PID_FILE /usr/share/${SERVICE_NAME}/agent.sh >/dev/null
killproc
Copy link
Member

Choose a reason for hiding this comment

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

Would you not pass an executable to killproc? Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

killproc is not the standard one. It's just a function in this file.

@adityasood adityasood added this to the Release 19.2.0 milestone Feb 25, 2019
stop-agent.sh knows how to find the pid, and uses kill to stop the
process
@ibnc
Copy link
Contributor Author

ibnc commented Feb 25, 2019

@ketan @arvindsv any idea why we weren't just using stop-agent.sh to stop agents? It looks like it does everything we want (including fixing the issue of pid location mentioned in the issue)

* Directly calling stop-agent.sh instead of wrapping in psuedo-killproc
fn
@marques-work marques-work merged commit 4713200 into gocd:master Feb 26, 2019
@ibnc ibnc self-assigned this Feb 26, 2019
@ketan
Copy link
Member

ketan commented Feb 27, 2019

Edit: I've made this change in d317486. Can someone see if that's allright?

Can we change the cat $PID_FILE | xargs kill to instead kill $(cat $PID_FILE). Reduces the need for having xargs as a dependency, because containers :)

ketan added a commit that referenced this pull request Feb 27, 2019
@ibnc
Copy link
Contributor Author

ibnc commented Feb 27, 2019

Edit: I've made this change in d317486. Can someone see if that's allright?

@ketan I see no issues with the change. It works as it should from what I can see

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.

5 participants