Opened 5 years ago

Closed 5 years ago

#3939 closed task (fixed)

Modify keatctrl to use server PID files

Reported by: tmark Owned by: tmark
Priority: low Milestone: Kea0.9.2
Component: scripts Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 12 Add Hours to Ticket: 0
Total Hours: 7 Internal?: no

Description

#3769, originally identified the fallibility of using "ps" to determine server PIDs in keactrl and called from an improved scheme. The accepted solution is to modify the servers to create PID files and then modify keactrl to use said PID files. I have split that effort into two tickets to make the work more manageable. Ticket #3769 covers the implementation of PID files in the servers. This ticket calls for keactrl to be modified to use them.

Subtickets

Change History (9)

comment:1 Changed 5 years ago by tmark

  • Feature Depending on Ticket set to #3939

comment:2 Changed 5 years ago by tmark

  • Feature Depending on Ticket #3939 deleted

comment:3 Changed 5 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:4 Changed 5 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 6

I replaced get_pids() with a new function, get_pid_from_file(), which constructs the PID file name and returns the PID contained therein or 0 if the file does not exist, as ${_pid}. It also returns the PID file name as ${_pid_file}.

The function, check_running(), uses this new function to determine if a server is running.

I found the lack of console output when stopping or reloading servers disconcerting. In order to improve feedback I replaced the use of send_signal() with two new functions, stop_server() and reload_server(). These functions now emit logs stating they are stopping/reloading the given server, or that that it isn't running.

I amended the log emitted by start_server when the server is already running to include the PID and PID file.

ChangeLog entry:

9xx.    [bug]       tmark
    keactrl now uses PID files to identify and control server instances.
    Prior to this it relied on the system command, "ps", which could lead
    to it misinterperting which processes are or are not running.
    (Trac #3939, git TBD)

comment:5 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:6 follow-up: Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

I reviewed commit 983cf57df5969f66de1d904e28c9147c21a4dd50

The Kea Guide's section "Chapter 6. Managing Kea with keactrl" should be updated to mention the use of pid files. Specifically, it should mention in what situations you need to manually delete the file and where this file is located, i.e. there is another process running having a pid from the pid file.

The use of KEA_PIDFILE_DIR should be mentioned in the Developer's Guide. I believe we have a "Unit-test" section which lists environment variables like this.

keactrl.in
I wonder if this log message is accurate:

        log_info "${binary_name} is already running as: \
+PID ${_pid}, PID file: ${_pid_file}."

It sounds like kea server is already running, while in some situations it may be another process which has hijacked the pid after server crash. It is a corner case really, but maybe just drop ${binary_name}?

The unit tests passed on FreeBSD10.1 and OS-X for me.

comment:7 in reply to: ↑ 6 Changed 5 years ago by tmark

  • Owner changed from tmark to marcin

Replying to marcin:

I reviewed commit 983cf57df5969f66de1d904e28c9147c21a4dd50

The Kea Guide's section "Chapter 6. Managing Kea with keactrl" should be updated to mention the use of pid files. Specifically, it should mention in what situations you need to manually delete the file and where this file is located, i.e. there is another process running having a pid from the pid file.

Done. I included more sample output for start, stop, and reload as well.

The use of KEA_PIDFILE_DIR should be mentioned in the Developer's Guide. I believe we have a "Unit-test" section which lists environment variables like this.

Done. I also alphabetized the list. Stephen would be so proud.

keactrl.in
I wonder if this log message is accurate:

        log_info "${binary_name} is already running as: \
+PID ${_pid}, PID file: ${_pid_file}."

It sounds like kea server is already running, while in some situations it may be another process which has hijacked the pid after server crash. It is a corner case really, but maybe just drop ${binary_name}?

I think having the binary_name there keeps the output consistent with other output
so I reworded the message to be

        log_info "${binary_name} appears to be running, see: \
PID ${_pid}, PID file: ${_pid_file}."

The unit tests passed on FreeBSD10.1 and OS-X for me.

Thanks for testing.


comment:8 Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit 3d701e67b8a08f8a337f00b21746f53cfbee6993

You're welcome to merge your changes!

comment:9 Changed 5 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 6 to 7

Changes merged with git 93a720ed7ffdffe66bd835cd64f78e4ad601637a
Added ChangeLog entry 981.

Ticket is complete.

Note: See TracTickets for help on using tickets.