Bug 185364

Created attachment 126079 [details]
cvs diff create_instance.[ch] instindex.cpp

Files:
ldap/admin/src/{create_instance.[ch], instindex.cpp}

Changes:
When ds_create is called with -r (update), scripts in the instance directory
were not updated.  They are recreated with this change.  The drawback is it's
applied every time the server is upgraded regardless of the necessity.	But I
think it's better to have a way to upgrade them than nothing...

Created attachment 126109 [details]
cvs commit message

Reviewed by Nathan and Rich (Thank you!)

Created attachment 126257 [details]
cvs diff create_instance.c and its commit message

Fix description:
I've found a bug in the code and some usage issue.

The bug is create_scripts was not returning NULL when the function finishes the
job successfully.

Index: create_instance.c
===================================================================
RCS file: /cvs/dirsec/ldapserver/ldap/admin/src/create_instance.c,v
retrieving revision 1.25
diff -t -w -U4 -r1.25 create_instance.c
--- create_instance.c	14 Mar 2006 17:13:38 -0000	1.25
+++ create_instance.c	16 Mar 2006 21:56:35 -0000
@@ -1059,9 +1059,9 @@

 static char *
 create_scripts(server_config_s *cf, char *param_name)
 {
-    char *t, *sroot = cf->sroot;
+    char *t = NULL, *sroot = cf->sroot;
     char subdir[PATH_SIZE];

     /* Create slapd-nickname directory */
     PR_snprintf(subdir, sizeof(subdir), "%s%c"PRODUCT_NAME"-%s", sroot,
FILE_PATHSEP,
@@ -1348,8 +1348,10 @@
     t = gen_script(subdir, RESTART_SCRIPT".bat", "net stop slapd-%s\n"
		    "net start slapd-%s\n", cf->servid, cf->servid);
     if(t) return t;
 #endif  /* XP_WIN32 */
+
+    return t; /* should be NULL */
 }

The usage problem to update the scripts is you have to pass install.inf for
each instance, if you have multiple.  That is, if you have slapd-test0,
slapd-test1, and slapd-test2, you have to have install.inf for each instance.

This is a sample install.inf for slapd-test1:
[General]
FullMachineName=   hostA.example.com
SuiteSpotUserID=   user1
SuitespotGroup=   group1
ServerRoot=   /path/to/ds
AdminDomain=   example.com
ConfigDirectoryAdminID=   admin
ConfigDirectoryAdminPwd=   password
ConfigDirectoryLdapURL=   ldap://hosta.example.com:55555/o=NetscapeRoot
UserDirectoryAdminID=	admin
UserDirectoryAdminPwd=	 password
UserDirectoryLdapURL=  
ldap://hosta.example.com:55555/dc=sfbay,dc=redhat,dc=com
Components=   svrcore,base,slapd,admin

[slapd]
SlapdConfigForMC=   Yes
SecurityOn=   No
UseExistingMC=	 No
UseExistingUG=	 No
ServerPort=   55556
ServerIdentifier=   test1
Suffix=   dc=example, dc=com
RootDN=   cn=Directory Manager
AddSampleEntries=   No
InstallLdifFile=   suggest
AddOrgEntries=	 Yes
DisableSchemaChecking=	 No
RootDNPwd=   password
Components=   slapd,slapd-client

As you could see, ds_create requires the instance specific info in the inf
file.  Unfortunately, ns-update cannot provide it automatically.  The upgrade
task would be something like this.

1) Prepare inf file for each server instance (say, install_instA.inf,
install_instB.inf, install_instC.inf, which should have the above [slapd]
section as well as [General].
2) Stop the servers
3) copy ds_create to <server_root>/bin/slapd/admin/bin
4) run <full_path>/ns-update -r -f install_instA.inf
5) for instance B and C
    cd <server_root>/bin/slapd/admin/bin; ./ds_create -r -f install_instB.inf 
    cd <server_root>/bin/slapd/admin/bin; ./ds_create -r -f install_instC.inf


Please note that ns-update should be run with the full path to ns-update, which
gives the utility the server root.

What was the original problem that this fix was trying to solve?  They needed to
update some information in each start-slapd in each instance?  Unless they
simply needed to recreate every single instance specific script from scratch
upon upgrade install, this could have much more easily been done with a simple
perl script called from ns-update:
if [ $reconfig ] ; then
	cd $sroot
	for dir in slapd-* ; do
....
We already have several perl scripts to do these sorts of things.  Or, in simple
cases, just use sed from ns-update.  Or, patch files.

Did HP get this patch?

(In reply to comment #4)
> What was the original problem that this fix was trying to solve?  They needed to
> update some information in each start-slapd in each instance?  

I'm attaching the original email from Grace.

> Unless they
> simply needed to recreate every single instance specific script from scratch
> upon upgrade install, this could have much more easily been done with a simple
> perl script called from ns-update:
> if [ $reconfig ] ; then
> 	cd $sroot
> 	for dir in slapd-* ; do
> ....
> We already have several perl scripts to do these sorts of things.  Or, in simple
> cases, just use sed from ns-update.  Or, patch files.
> 
> Did HP get this patch?

I believe so.  Grace wrote: "I back ported your changes to ds 6.11."

Created attachment 138349 [details]
original email from Grace

Created attachment 138351 [details]
diffs for ns-update

In order to make ns-update work with upgrade install, it needs to grab the
missing information and add it to the install.inf passed to ds_create -r.

Hi Rich.  Your changes look good.  I have one question.  Why you chose
"o=NetscapeRoot" for Suffix?  It's applied to all the instances, isn't it? 
Simply because it's harmless?
+    suffix="o=NetscapeRoot"

I haven't worked on FHS in newinst, yet... :p  It'll be in my phase 3...
+    dseldif=$sroot/$dir/config/dse.ldif

The value of suffix is ignored during reconfig, but the code still checks to
make sure a suffix was given and that it is a valid DN.  So I just chose
o=NetscapeRoot because it is safe for this purpose.

Re: the FHS stuff - yeah, most of this setup type stuff is going to have to
change . . .

Created attachment 138364 [details]
cvs commit log

Reviewed by: nhosoi (Thanks!)
Files: ldapserver/ldap/cm/newinst/ns-update
Branch: HEAD
Fix Description: ns-update needs to run ds_create -r for each instance in the
server root because the start-slapd et. al. scripts that it needs to recreate
are per-instance.  The install.inf passed in from setup only contains the
information in the [General] and [admin] section.  We need to supply the
missing information for the [slapd] section to make both create_scripts() and
reconfigure_instance() happy.
Platforms tested: RHEL4
Flag Day: no
Doc impact: no

After my discussion with Noriko, we think it makes sense to do the followings:
1. Mark this bug as NOT_A_BUG.
2. Open a new one for DS8.1