Bug 90 - RFE: Add a "create" method to pbs_server init.d script
: RFE: Add a "create" method to pbs_server init.d script
Status: RESOLVED FIXED
Product: TORQUE
pbs_server
: 2.5.x
: PC Linux
: P5 enhancement
Assigned To: Glen
:
:
:
  Show dependency treegraph
 
Reported: 2010-10-14 15:10 MDT by Steve Traylen
Modified: 2011-02-01 12:33 MST (History)
4 users (show)

See Also:


Attachments
Adds a create method to the pbs_server init.d script. (1.29 KB, patch)
2010-10-14 15:10 MDT, Steve Traylen
Details | Diff
The Scyld ClusterWare shipped version (5.64 KB, text/plain)
2010-12-01 13:21 MST, Joshua Bernstein
Details
Proposed init script patch (2.65 KB, patch)
2010-12-01 18:32 MST, Michael Jennings
Details | Diff
Updated proposed patch (2.72 KB, patch)
2011-01-21 11:53 MST, Michael Jennings
Details | Diff
Updated proposed patch (2.90 KB, patch)
2011-01-24 13:19 MST, Michael Jennings
Details | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description Steve Traylen 2010-10-14 15:10:41 MDT
Created an attachment (id=57) [details]
Adds a create method to the pbs_server init.d script.

This was requested here:

https://bugzilla.redhat.com/show_bug.cgi?id=631256

if this patch is to be added is a matter of opinion for sure.

The attached patch does not create a serverdb file if it does
exist but instead just suggests to create with a 

service pbs_server create 

and that method has been implemented also by this patch.
Comment 1 Glen 2010-11-30 20:20:09 MST
I would not be opposed to this patch, any other opinions?
Comment 2 Chris Samuel 2010-11-30 22:24:52 MST
Sounds sensible to me.
Comment 3 Ken Nielson 2010-12-01 08:59:49 MST
I will defer to the administrators who use this. I do not use init.d to start
the server. But I do not see any problems at first glance to add a create
option. The create looks for an existing serverdb and does not overwrite it. It
would only be useful if a serverdb does not exist.
Comment 4 Michael Jennings 2010-12-01 11:58:53 MST
With the addition of the -d $PBS_HOME to the pbs_server startup, the "create"
operation has become far less dangerous than it was when dbeer originally
commented it out.  It seems to me that maybe, in addition to adding a "create"
option, create should still be the default if no serverdb is present in
$PBS_HOME.

I'm a big fan of getting as close to working-out-of-the-box as possible, and
automatic creation of a basic configuration when nothing exists is a good means
to that end, IMHO.

I can create an alternative patch that crystallizes my thoughts if others
agree.
Comment 5 Joshua Bernstein 2010-12-01 13:21:43 MST
Created an attachment (id=64) [details]
The Scyld ClusterWare shipped version

You might find the init script we use with Scyld to be a bit more complete. It
also includes things like (configure) and (reconfigure). Though the
config-nodes action used some Scyld Specific stuff and probably should be
stripped out.

-Josh
Comment 6 Michael Jennings 2010-12-01 13:39:08 MST
(In reply to comment #5)
> You might find the init script we use with Scyld to be a bit more complete. It
> also includes things like (configure) and (reconfigure). Though the
> config-nodes action used some Scyld Specific stuff and probably should be
> stripped out.

IMHO, this script would take a LOT of work to make usable.  It violates several
principles of init scripts (such as not calling other init scripts and sourcing
external configuration) and makes numerous assumptions about the cluster
configuration.

I saw a couple ideas in this script that might be good to add in to the
existing init script, but starting with this one would take far more work.
Comment 7 Steve Traylen 2010-12-01 13:43:56 MST
One of the motivations for not doing a create within the start method when
the db does not exist is the following.

# rm -f /var/lib/torque/server_priv/serverdb
# /etc/init.d/pbs_server start
Starting TORQUE Server:                                    [  OK  ]

Of course we just created the serverdb at this point.

# pgrep -fl pbs_server
30282 /usr/sbin/pbs_server -t create

To me it some how seems a bit messy to leave the process running with
this extra "-t create" the first time.

It just seems a bit inconsistent somehow to start the service in a different
way depending on the starting state.

Steve.
Comment 8 Michael Jennings 2010-12-01 18:32:00 MST
Created an attachment (id=65) [details]
Proposed init script patch

Here's a possible patch which would take your concerns into account as well as
mine.  Thoughts?
Comment 9 Michael Jennings 2011-01-04 11:44:11 MST
(In reply to comment #8)
> Created an attachment (id=65) [details] [details]
> Proposed init script patch
> 
> Here's a possible patch which would take your concerns into account as well as
> mine.  Thoughts?

If no one is screaming, "NO!" does that imply a "Yes?"  :-)
Comment 10 Ken Nielson 2011-01-11 10:58:20 MST
This is to second Michael's call for comment. Please review the patch and let
us know if there are any objections. If now we will submit this to the build.

Ken Nielson
Comment 11 Chris Samuel 2011-01-11 20:30:57 MST
One thought on the create option - currently it just blows any existing DB
away.

My suggestion is for it to check if one exists and if so warn the admin and ask
them to remove it, then exit with a non-zero value.

Just in case...
Comment 12 Steve Traylen 2011-01-12 01:03:12 MST
(In reply to comment #11)
> One thought on the create option - currently it just blows any existing DB
> away.
> 
> My suggestion is for it to check if one exists and if so warn the admin and ask
> them to remove it, then exit with a non-zero value.
> Just in case...

Yes that would be my preference also, make it hard to do damage.

Steve.
Comment 13 Michael Jennings 2011-01-12 01:26:18 MST
(In reply to comment #11)
> One thought on the create option - currently it just blows any existing DB
> away.
> 
> My suggestion is for it to check if one exists and if so warn the admin and ask
> them to remove it, then exit with a non-zero value.

It is not permissible for an init script to ask questions.

Besides, if you read up, Ken already said that "create" will not overwrite an
existing DB.  So what's the problem here?
Comment 14 Michael Jennings 2011-01-21 11:53:56 MST
Created an attachment (id=70) [details]
Updated proposed patch

This new patch contains all the previous changes and adds the ability to
specify additional arguments to pbs_server in the /etc/sysconfig/pbs_server
file.  This allows an admin to, for example, set PBS_ARGS="--ha" in the
aforementioned file and have it persistent across reinstalls of the torque
RPM's.  Previously one had to edit the init script directly, and those changes
would be overwritten by newer torque packages (since the init script is not
marked as a config file, nor should it be).

If anyone is opposed to either or both changes, please speak up.
Comment 15 Glen 2011-01-21 12:07:43 MST
(In reply to comment #14)
> Created an attachment (id=70) [details] [details]
> Updated proposed patch
> 
> This new patch contains all the previous changes and adds the ability to
> specify additional arguments to pbs_server in the /etc/sysconfig/pbs_server
> file.  This allows an admin to, for example, set PBS_ARGS="--ha" in the
> aforementioned file and have it persistent across reinstalls of the torque
> RPM's.  Previously one had to edit the init script directly, and those changes
> would be overwritten by newer torque packages (since the init script is not
> marked as a config file, nor should it be).
> 
> If anyone is opposed to either or both changes, please speak up.


I like the changes
Comment 16 Chris Samuel 2011-01-23 16:19:58 MST
@Michael - I wasn't proposing that the script ask a question, just tell the
user the DB has already been initialised if it exists.

Currently your changes will just blow any existing configuration away as it
does not test for its existence (unless I'm missing something).
Comment 17 Michael Jennings 2011-01-24 13:19:29 MST
Created an attachment (id=71) [details]
Updated proposed patch

Updated patch to check for serverdb and fail create() if present
Comment 18 Chris Samuel 2011-01-24 15:32:25 MST
Thanks Michael, the change to create looks good to me!

One query - any reason to use killproc rather than qterm -t quick ?
Comment 19 Ken Nielson 2011-02-01 12:33:36 MST
This patch has been added to TORQUE 2.5-fixes, 3.0-fixes and trunk