PR#50213: Ticket 50208 - make instances mark off based on dse.ldif not sysconfig - 389-ds-base

389-ds-base

#50213 Ticket 50208 - make instances mark off based on dse.ldif not sysconfig

Closed 3 years ago by spichugi. Opened 5 years ago by firstyear.

firstyear/389-ds-base 50208-detect-lib389-dse-ldif into master

Ticket 50208 - make instances mark off based on dse.ldif not sysconfig

William Brown • 5 years ago

e373f39

Makefile.am

file modified

-10

		`@@ -647,7 +647,6 @@`
		`$(srcdir)/LICENSE.* \`
		`$(srcdir)/VERSION.sh \`
		`$(srcdir)/wrappers/*.in \`
		`- $(srcdir)/wrappers/systemd.template.sysconfig \`
		`$(srcdir)/dirsrvtests \`
		`$(srcdir)/src/lib389/setup.py \`
		`$(srcdir)/src/lib389`
		`@@ -900,14 +899,9 @@`
		`endif`
		`endif`

		`- if SYSTEMD`
		`- initconfig_DATA = ldap/admin/src/$(PACKAGE_NAME) \`
		`- wrappers/$(PACKAGE_NAME).systemd`
		`- else`
		`if INITDDIR`
		`initconfig_DATA = ldap/admin/src/$(PACKAGE_NAME)`
		`endif`
		`- endif`

		`inf_DATA = ldap/admin/src/slapd.inf \`
		`ldap/admin/src/scripts/dscreate.map \`
		`@@ -2342,10 +2336,6 @@`
		`fi; \`
		`$(fixupcmd) $$service_template > $@`

		`- %/$(PACKAGE_NAME).systemd: %/systemd.template.sysconfig`
		`- if [ ! -d $(dir $@) ] ; then mkdir -p $(dir $@) ; fi`
		`- $(fixupcmd) $^ > $@`
		`-`
		`%/$(systemdgroupname): %/systemd.group.in`
		`if [ ! -d $(dir $@) ] ; then mkdir -p $(dir $@) ; fi`
		`$(fixupcmd) $^ > $@`

rpm/389-ds-base.spec.in

file modified

-2

		`@@ -599,8 +599,6 @@`
		`%config(noreplace)%{_sysconfdir}/%{pkgname}/config/slapd-collations.conf`
		`%config(noreplace)%{_sysconfdir}/%{pkgname}/config/certmap.conf`
		`%config(noreplace)%{_sysconfdir}/%{pkgname}/config/template-initconfig`
		`- %config(noreplace)%{_sysconfdir}/sysconfig/%{pkgname}`
		`- %config(noreplace)%{_sysconfdir}/sysconfig/%{pkgname}.systemd`
		`%{_datadir}/%{pkgname}`
		`%exclude %{_datadir}/%{pkgname}/script-templates`
		`%exclude %{_datadir}/%{pkgname}/updates`

src/lib389/lib389/__init__.py

file modified

+44 -164

		`@@ -601,10 +601,7 @@`
		`def list(self, all=False, serverid=None):`
		`"""`
		`Returns a list dictionary. For a created instance that is on the`
		`- local file system (e.g. <prefix>/etc/dirsrv/slapd-*), it exists`
		`- a file describing its properties`
		`- (environment): <prefix>/etc/sysconfig/dirsrv-<serverid> or`
		`- $HOME/.dirsrv/dirsv-<serverid>`
		`+ local file system (e.g. <prefix>/etc/dirsrv/slapd-*/dse.ldif).`
		`A dictionary is created with the following properties:`
		`CONF_SERVER_DIR`
		`CONF_SERVERBIN_DIR`
		`@@ -628,16 +625,8 @@`
		`@raise IOError - if the file containing the properties is not`
		`foundable or readable`
		`"""`
		`- def test_and_set(prop, propname, variable, value):`
		`- '''`
		`- If variable is 'propname' it adds to`
		`- 'prop' dictionary the propname:value`
		`- '''`
		`- if variable == propname:`
		`- prop[propname] = value`
		`- return 1`
		`- return 0`

		`+ ### This inner function WILL BE REMOVED soon.`
		`def _parse_configfile(filename=None, serverid=None):`
		`'''`
		`This method read 'filename' and build a dictionary with`
		`@@ -653,46 +642,24 @@`
		`prop = {}`
		`prop[CONF_SERVER_ID] = serverid`
		`prop[SER_SERVERID_PROP] = serverid`
		`- myfile = open(filename, 'r')`
		`- for line in myfile:`
		`- # retrieve the value in line::`
		`- # <PROPNAME>=<string> [';' export <PROPNAME>]`
		`-`
		`- # skip comment lines`
		`- if line.startswith('#'):`
		`- continue`
		`-`
		`- # skip lines without assignment`
		`- if '=' not in line:`
		`- continue`
		`- value = line.split(';', 1)[0]`
		`-`
		`- # skip lines without assignment`
		`- if '=' not in value:`
		`- continue`

		`- variable = value.split('=', 1)[0]`
		`- value = value.split('=', 1)[1]`
		`- value = value.strip(' \t\n')`
		`- for property in (CONF_SERVER_DIR,`
		`- CONF_SERVERBIN_DIR,`
		`- CONF_CONFIG_DIR,`
		`- CONF_INST_DIR,`
		`- CONF_RUN_DIR,`
		`- CONF_DS_ROOT,`
		`- CONF_PRODUCT_NAME):`
		`- if test_and_set(prop, property, variable, value):`
		`- break`
		`-`
		`- # Now, we have passed the sysconfig environment file.`
		`- # read in and parse the dse.ldif to determine our SER_* values.`
		`- # probably should use path join?`
		`- dsefile = '%s/dse.ldif' % prop[CONF_CONFIG_DIR]`
		`- if os.path.exists(dsefile):`
		`- ldifconn = LDIFConn(dsefile)`
		`- configentry = ldifconn.get(DN_CONFIG)`
		`- for key in args_dse_keys:`
		`- prop[key] = configentry.getValue(args_dse_keys[key])`
		`+ inst_paths = Paths(serverid)`
		`+`
		`+ # WARNING: This is not correct, but is a stop gap until: https://pagure.io/389-ds-base/issue/50207`
		`+ # Once that's done, this will "just work". Saying this, the whole prop dictionary`
		`+ # concept is fundamentally broken, and we should be using ds_paths anyway.`
		`+ prop[CONF_SERVER_DIR] = inst_paths.lib_dir`
		`+ prop[CONF_SERVERBIN_DIR] = inst_paths.sbin_dir`
		`+ prop[CONF_CONFIG_DIR] = inst_paths.config_dir`
		`+ prop[CONF_INST_DIR] = inst_paths.inst_dir`
		`+ prop[CONF_RUN_DIR] = inst_paths.run_dir`
		`+ prop[CONF_DS_ROOT] = ''`
		`+ prop[CONF_PRODUCT_NAME] = 'slapd'`
		`+`
		`+ ldifconn = LDIFConn(filename)`
		`+ configentry = ldifconn.get(DN_CONFIG)`
		`+ for key in args_dse_keys:`
		`+ prop[key] = configentry.getValue(args_dse_keys[key])`
		`# SER_HOST (host) nsslapd-localhost`
		`# SER_PORT (port) nsslapd-port`
		`# SER_SECURE_PORT (sslport) nsslapd-secureport`
		`@@ -702,73 +669,19 @@`
		`# nsslapd-defaultnamingcontext`
		`# SER_USER_ID (userid) nsslapd-localuser`
		`# SER_SERVERID_PROP (serverid) Already have this`
		`- # SER_GROUP_ID (groupid) ???`
		`+ # SER_GROUP_ID (groupid)`
		`# SER_DEPLOYED_DIR (prefix) Already provided to for`
		`# discovery`
		`- # SER_BACKUP_INST_DIR (backupdir) nsslapd-bakdir <<-- maybe?`
		`+ # SER_BACKUP_INST_DIR (backupdir) nsslapd-bakdir`
		`# We need to convert these two to int`
		`# because other routines get confused if we don't`
		`for intkey in [SER_PORT, SER_SECURE_PORT]:`
		`- if prop[intkey] is not None:`
		`+ if intkey in prop and prop[intkey] is not None:`
		`prop[intkey] = int(prop[intkey])`
		`return prop`
		`+ ### end _parse_configfile`

		`- def search_dir(instances, pattern, stop_value=None):`
		`- '''`
		`- It search all the files matching pattern.`
		`- It there is not stop_value, it adds the properties found in`
		`- each file to the 'instances'`
		`- Else it searches the specific stop_value (instance's serverid)`
		`- to add only its properties in the 'instances'`
		`-`
		`- @param instances - list of dictionary containing the instances`
		`- properties`
		`- @param pattern - pattern to find the files containing the`
		`- properties`
		`- @param stop_value - serverid value if we are looking only for`
		`- one specific instance`
		`-`
		`- @return True or False - If stop_value is None it returns False.`
		`- If stop_value is specified, it returns`
		`- True if it added the property`
		`- dictionary in instances. Or False if it`
		`- did not find it.`
		`- '''`
		`- added = False`
		`- for instance in glob.glob(pattern):`
		`- serverid = os.path.basename(instance)[len(DEFAULT_ENV_HEAD):]`
		`-`
		`- # skip removed instance and admin server entry`
		`- if '.removed' in serverid or 'dirsrv-admin' in instance:`
		`- continue`
		`-`
		`- # it is found, store its properties in the list`
		`- if stop_value:`
		`- if stop_value == serverid:`
		`- instances.append(_parse_configfile(instance, serverid))`
		`- added = True`
		`- break`
		`- else:`
		`- # this is not the searched value, continue`
		`- continue`
		`- else:`
		`- # we are not looking for a specific value, just add it`
		`- instances.append(_parse_configfile(instance, serverid))`
		`-`
		`- return added`
		`-`
		`- # Retrieves all instances under '/etc/sysconfig' and '/etc/dirsrv'`
		`-`
		`- # Instances/Environment are`
		`- #`
		`- # file: /etc/sysconfig/dirsrv-<serverid> (env)`
		`- # inst: /etc/dirsrv/slapd-<serverid> (conf)`
		`- #`
		`- # or`
		`- #`
		`- # file: $HOME/.dirsrv/dirsrv-<serverid> (env)`
		`- # inst: <prefix>/etc/dirsrv/slapd-<serverid> (conf)`
		`- #`
		`+ # Retrieves all instances under '<prefix>/etc/dirsrv'`

		`# Don't need a default value now since it's set in init.`
		`if serverid is None and hasattr(self, 'serverid'):`
		`@@ -776,68 +689,35 @@`
		`elif serverid is not None:`
		`serverid = serverid.replace('slapd-', '')`

		`- # first identify the directories we will scan`
		`- sysconfig_head = self.ds_paths.initconfig_dir`
		`- privconfig_head = os.path.expanduser(os.path.join('~', ENV_LOCAL_DIR))`
		`- if not os.path.isdir(sysconfig_head):`
		`- privconfig_head = None`
		`- self.log.debug("dir (sys) : %s", sysconfig_head)`
		`- if privconfig_head:`
		`- self.log.debug("dir (priv): %s", privconfig_head)`
		`-`
		`# list of the found instances`
		`instances = []`

		`# now prepare the list of instances properties`
		`if not all:`
		`+ dse_ldif = os.path.join(self.ds_paths.config_dir, 'dse.ldif')`
		`# easy case we just look for the current instance`
		`-`
		`- # we have two location to retrieve the self.serverid`
		`- # privconfig_head and sysconfig_head`
		`-`
		`- # first check the private repository`
		`- if privconfig_head:`
		`- pattern = "%s*" % os.path.join(privconfig_head,`
		`- DEFAULT_ENV_HEAD)`
		`- found = search_dir(instances, pattern, serverid)`
		`- if len(instances) > 0:`
		`- self.log.debug("List from %s", privconfig_head)`
		`- for instance in instances:`
		`- self.log.debug("list instance %r\n", instance)`
		`- if found:`
		`- assert len(instances) == 1`
		`- else:`
		`- assert len(instances) == 0`
		`+ if os.path.exists(dse_ldif):`
		`+ # It's real`
		`+ # Now just populate that instance dict (soon to be changed ...)`
		`+ instances.append(_parse_configfile(dse_ldif, serverid))`
		`else:`
		`- found = False`
		`-`
		`- # second, if not already found, search the system repository`
		`- if not found:`
		`- pattern = "%s*" % os.path.join(sysconfig_head,`
		`- DEFAULT_ENV_HEAD)`
		`- search_dir(instances, pattern, serverid)`
		`- if len(instances) > 0:`
		`- self.log.debug("List from %s", privconfig_head)`
		`- for instance in instances:`
		`- self.log.debug("list instance %r\n", instance)`
		`+ # it's not`
		`+ self.log.debug("list instance not found: %s\n", serverid)`

		`else:`
		`- # all instances must be retrieved`
		`- if privconfig_head:`
		`- pattern = "%s*" % os.path.join(privconfig_head,`
		`- DEFAULT_ENV_HEAD)`
		`- search_dir(instances, pattern)`
		`- if len(instances) > 0:`
		`- self.log.debug("List from %s", privconfig_head)`
		`- for instance in instances:`
		`- self.log.debug("list instance %r\n", instance)`
		`-`
		`- pattern = "%s*" % os.path.join(sysconfig_head, DEFAULT_ENV_HEAD)`
		`- search_dir(instances, pattern)`
		`- if len(instances) > 0:`
		`- self.log.debug("List from %s", privconfig_head)`
		`- for instance in instances:`
		`- self.log.debug("list instance %r\n", instance)`
		`+ # For each dir that starts with slapd-*`
		`+ potential_inst = [`
		`+ f for f`
		`+ in os.listdir(self.ds_paths.sysconf_dir)`
		`+ if os.path.isdir(f) and f.startswith('slapd-')]`
		`+ # check it has dse.ldif`
		`+ for pi in potential_inst:`
		`+ pi_dse_ldif = os.path.join(potential_inst, 'dse.ldif')`
		`+ # Takes /etc/dirsrv/slapd-instance -> slapd-instance -> instance`
		`+ pi_name = pi.split('/')[-1].split('-')[-1]`
		`+ # parse + append`
		`+ if os.path.exists(pi_dse_ldif):`
		`+ instances.append(_parse_configfile(pi_dse_ldif, pi_name))`

		`return instances`

src/lib389/lib389/instance/remove.py

file modified

+70 -43

		`@@ -9,12 +9,30 @@`
		`import os`
		`import shutil`
		`import subprocess`
		`- from lib389.utils import selinux_label_port`
		`+ from lib389.nss_ssl import NssSsl`
		`+ from lib389.utils import selinux_label_port, assert_c`
		`+`
		`+`
		`+ ######################## WARNING #############################`
		`+ # DO NOT CHANGE THIS FILE OR ITS CONTENTS WITHOUT READING`
		`+ # ALL OF THE COMMENTS FIRST. THERE ARE VERY DELICATE`
		`+ # AND DETAILED INTERACTIONS OF COMPONENTS IN THIS FILE.`
		`+ #`
		`+ # IF IN DOUBT CONTACT WILLIAM BROWN <william@blackhats.net.au>`


		`def remove_ds_instance(dirsrv, force=False):`
		`"""`
		`- This will delete the instance as it is define. This must be a local instance.`
		`+ This will delete the instance as it is define. This must be a local instance. This is`
		`+ designed to raise exceptions quickly and often if any error is hit. However, this can`
		`+ be run repeatedly, and only when the instance is truely removed, will this program fail`
		`+ to run further.`
		`+`
		`+ :param dirsrv: A directory server instance`
		`+ :type dirsrv: DirSrv`
		`+ :param force: A psycological aid, for people who think force means do something, harder. Does`
		`+ literally nothing in this program because state machines are a thing.`
		`+ :type force: bool`
		`"""`
		`_log = dirsrv.log.getChild('remove_ds')`
		`_log.debug("Removing instance %s" % dirsrv.serverid)`
		`@@ -38,30 +56,22 @@`
		`# remove_paths['run_dir'] = dirsrv.ds_paths.run_dir`
		`remove_paths['tmpfiles_d'] = dirsrv.ds_paths.tmpfiles_d + "/dirsrv-" + dirsrv.serverid + ".conf"`
		`remove_paths['inst_dir'] = dirsrv.ds_paths.inst_dir`
		`+ remove_paths['etc_sysconfig'] = "%s/sysconfig/dirsrv-%s" % (dirsrv.ds_paths.sysconf_dir, dirsrv.serverid)`

		`- marker_path = "%s/sysconfig/dirsrv-%s" % (dirsrv.ds_paths.sysconf_dir, dirsrv.serverid)`
		`+ # These are handled in a special way.`
		`etc_dirsrv_path = os.path.join(dirsrv.ds_paths.sysconf_dir, 'dirsrv/')`
		`- ssca_path = os.path.join(etc_dirsrv_path, 'ssca/')`
		`+ dse_ldif_path = os.path.join(etc_dirsrv_path, 'dse.ldif')`

		`# Check the marker exists. If it does not warn about this, and say that to`
		`# force removal you should touch this file.`

		`- _log.debug("Checking for instance marker at %s" % marker_path)`
		`- assert os.path.exists(marker_path)`
		`-`
		`- # Move the config_dir to config_dir.removed`
		`- config_dir = dirsrv.ds_paths.config_dir`
		`- config_dir_rm = "{}.removed".format(config_dir)`
		`-`
		`- if os.path.exists(config_dir_rm):`
		`- _log.debug("Removing previously existed %s" % config_dir_rm)`
		`- shutil.rmtree(config_dir_rm)`
		`+ _log.debug("Checking for instance marker at %s" % dse_ldif_path)`
		`+ if not os.path.exists(dse_ldif_path):`
		`+ _log.info("Instance configuration not found, no action will be taken")`
		`+ _log.info("If you want us to cleanup anyway, recreate '%s'" % dse_ldif_path)`
		`+ return`

		`- _log.debug("Copying %s to %s" % (config_dir, config_dir_rm))`
		`- try:`
		`- shutil.copytree(config_dir, config_dir_rm)`
		`- except FileNotFoundError:`
		`- pass`
		`+ ### ANY NEW REMOVAL ACTION MUST BE BELOW THIS LINE!!!`

		`# Remove these paths:`
		`# for path in ('backup_dir', 'cert_dir', 'config_dir', 'db_dir',`
		`@@ -73,36 +83,53 @@`
		`# Remove parent (/var/lib/dirsrv/slapd-INST)`
		`shutil.rmtree(remove_paths['db_dir'].replace('db', ''), ignore_errors=True)`

		`- # Finally remove the sysconfig marker.`
		`- os.remove(marker_path)`
		`- _log.debug("Removing %s" % marker_path)`
		`+ # We can not assume we have systemd ...`
		`+ if dirsrv.ds_paths.with_systemd:`
		`+ # Remove the systemd symlink`
		`+ _log.debug("Removing the systemd symlink")`
		`+ subprocess.check_call(["systemctl", "disable", "dirsrv@{}".format(dirsrv.serverid)])`

		`- # Remove the systemd symlink`
		`- _log.debug("Removing the systemd symlink")`
		`- subprocess.check_call(["systemctl", "disable", "dirsrv@{}".format(dirsrv.serverid)])`
		`-`
		`- # Remove selinux port label`
		`- _log.debug("Removing the port label")`
		`- try:`
		`+ # Nor can we assume we have selinux. Try docker sometime ;)`
		`+ if dirsrv.ds_paths.with_selinux:`
		`+ # Remove selinux port label`
		`+ _log.debug("Removing the port labels")`
		`selinux_label_port(dirsrv.port, remove_label=True)`
		`- except ValueError as e:`
		`- if force:`
		`- pass`
		`- else:`
		`- _log.error(str(e))`
		`- raise e`

		`- if dirsrv.sslport is not None:`
		`- selinux_label_port(dirsrv.sslport, remove_label=True)`
		`+ # This is a compatability with ancient installs, all modern install have tls port`
		`+ if dirsrv.sslport is not None:`
		`+ selinux_label_port(dirsrv.sslport, remove_label=True)`

		`- # If this was the last instance, remove the ssca directory`
		`+ # If this was the last instance, remove the ssca instance`
		`insts = dirsrv.list(all=True)`
		`if len(insts) == 0:`
		`- # Remove /etc/dirsrv/ssca`
		`- try:`
		`- shutil.rmtree(ssca_path)`
		`- except FileNotFoundError:`
		`- pass`
		`+ ssca = NssSsl(dbpath=dirsrv.get_ssca_dir())`
		`+ ssca.remove_db()`
		`+`
		`+ ### ANY NEW REMOVAL ACTIONS MUST BE ABOVE THIS LINE!!!`
		`+`
		`+ # Finally means FINALLY, the last thing, the LAST LAST thing. By doing this absolutely`
		`+ # last, it means that we can have any failure above, and continue to re-run until resolved`
		`+ # because this instance marker (dse.ldif) continues to exist!`
		`+ # Move the config_dir to config_dir.removed`
		`+ config_dir = dirsrv.ds_paths.config_dir`
		`+ config_dir_rm = "{}.removed".format(config_dir)`
		`+`
		`+ if os.path.exists(config_dir_rm):`
		`+ _log.debug("Removing previously existed %s" % config_dir_rm)`
		`+ shutil.rmtree(config_dir_rm)`
		`+`
		`+ assert_c(not os.path.exists(config_dir_rm))`
		`+`
		`+ # That's it, everything before this MUST have suceeded, so now we can move the`
		`+ # config dir (containing dse.ldif, the marker) out of the way.`
		`+ _log.debug("Moving %s to %s" % (config_dir, config_dir_rm))`
		`+ try:`
		`+ shutil.move(config_dir, config_dir_rm)`
		`+ except FileNotFoundError:`
		`+ pass`
		`+`
		`+ # DO NOT PUT ANY CODE BELOW THIS COMMENT BECAUSE THAT WOULD VIOLATE THE ASSERTIONS OF THE`
		`+ # ABOVE CODE.`

		`# Done!`
		`_log.debug("Complete")`

src/lib389/lib389/instance/setup.py

file modified

+61 -65

		`@@ -654,37 +654,72 @@`
		`"""`
		`Actually install the Ds from the dicts provided.`

		`- You should never call this directly, as it bypasses assert_cions.`
		`+ You should never call this directly, as it bypasses assertions.`
		`"""`
		`- # register the instance to /etc/sysconfig`
		`- # We do this first so that we can trick remove-ds.pl if needed.`
		`- # There may be a way to create this from template like the dse.ldif ...`
		`- initconfig = ""`
		`- with open("%s/dirsrv/config/template-initconfig" % slapd['sysconf_dir']) as template_init:`
		`- for line in template_init.readlines():`
		`- initconfig += line.replace('{{', '{', 1).replace('}}', '}', 1).replace('-', '_')`
		`+ ######################## WARNING #############################`
		`+ # DO NOT CHANGE THIS FUNCTION OR ITS CONTENTS WITHOUT READING`
		`+ # ALL OF THE COMMENTS FIRST. THERE ARE VERY DELICATE`
		`+ # AND DETAILED INTERACTIONS OF COMPONENTS IN THIS FUNCTION.`
		`+ #`
		`+ # IF IN DOUBT CONTACT WILLIAM BROWN <william@blackhats.net.au>`
		`+`
		`+`
		`+ ### This first section is about creating the minimal required paths and config to get`
		`+ # directory server to start: After this, we then perform all configuration as online`
		`+ # changes from after this point.`
		`+`
		`+ # Create dse.ldif with a temporary root password.`
		`+ # This is done first, because instances are found for removal and listing by detecting`
		`+ # the present of their dse.ldif!!!!`
		`+ # The template is in slapd['data_dir']/dirsrv/data/template-dse.ldif`
		`+ # Variables are done with %KEY%.`
		`+ self.log.debug("ACTION: Creating dse.ldif")`
		`try:`
		`- # /etc/sysconfig`
		`- os.makedirs("%s" % slapd['initconfig_dir'], mode=0o770)`
		`- except FileExistsError:`
		`+ os.umask(0o007) # For parent dirs that get created -> sets 770 for perms`
		`+ os.makedirs(slapd['config_dir'], mode=0o770)`
		`+ except OSError:`
		`pass`
		`- sysconfig_filename = "%s/dirsrv-%s" % (slapd['initconfig_dir'], slapd['instance_name'])`
		`- with open(sysconfig_filename, 'w') as f:`
		`- f.write(initconfig.format(`
		`- SERVER_DIR=slapd['lib_dir'],`
		`- SERVERBIN_DIR=slapd['sbin_dir'],`
		`- CONFIG_DIR=slapd['config_dir'],`
		`- INST_DIR=slapd['inst_dir'],`
		`- RUN_DIR=slapd['run_dir'],`
		`- DS_ROOT='',`
		`- PRODUCT_NAME='slapd',`
		`+`
		`+ # Get suffix for some plugin defaults (if possible)`
		`+ # annoyingly for legacy compat backend takes TWO key types`
		`+ # and we have to now deal with that ....`
		`+ #`
		`+ # Create ds_suffix here else it won't be in scope ....`
		`+ ds_suffix = ''`
		`+ if len(backends) > 0:`
		`+ ds_suffix = normalizeDN(backends[0]['nsslapd-suffix'])`
		`+`
		`+ dse = ""`
		`+ with open(os.path.join(slapd['data_dir'], 'dirsrv', 'data', 'template-dse.ldif')) as template_dse:`
		`+ for line in template_dse.readlines():`
		`+ dse += line.replace('%', '{', 1).replace('%', '}', 1)`
		`+`
		`+ with open(os.path.join(slapd['config_dir'], 'dse.ldif'), 'w') as file_dse:`
		`+ file_dse.write(dse.format(`
		`+ schema_dir=slapd['schema_dir'],`
		`+ lock_dir=slapd['lock_dir'],`
		`+ tmp_dir=slapd['tmp_dir'],`
		`+ cert_dir=slapd['cert_dir'],`
		`+ ldif_dir=slapd['ldif_dir'],`
		`+ bak_dir=slapd['backup_dir'],`
		`+ run_dir=slapd['run_dir'],`
		`+ inst_dir=slapd['inst_dir'],`
		`+ log_dir=slapd['log_dir'],`
		`+ fqdn=general['full_machine_name'],`
		`+ ds_port=slapd['port'],`
		`+ ds_user=slapd['user'],`
		`+ rootdn=slapd['root_dn'],`
		`+ ds_passwd=self._secure_password, # We set our own password here, so we can connect and mod.`
		`+ # This is because we never know the users input root password as they can validily give`
		`+ # us a hashed input.`
		`+ ds_suffix=ds_suffix,`
		`+ config_dir=slapd['config_dir'],`
		`+ db_dir=slapd['db_dir'],`
		`))`
		`- os.chmod(sysconfig_filename, 0o440)`
		`- os.chown(sysconfig_filename, slapd['user_uid'], slapd['group_gid'])`

		`# Create all the needed paths`
		`# we should only need to make bak_dir, cert_dir, config_dir, db_dir, ldif_dir, lock_dir, log_dir, run_dir?`
		`- for path in ('backup_dir', 'cert_dir', 'config_dir', 'db_dir', 'ldif_dir', 'lock_dir', 'log_dir', 'run_dir'):`
		`+ for path in ('backup_dir', 'cert_dir', 'db_dir', 'ldif_dir', 'lock_dir', 'log_dir', 'run_dir'):`
		`self.log.debug("ACTION: creating %s", slapd[path])`
		`try:`
		`os.umask(0o007) # For parent dirs that get created -> sets 770 for perms`
		`@@ -745,51 +780,12 @@`

		`# Bind sockets to our type?`

		`- # Get suffix for some plugin defaults (if possible)`
		`- # annoyingly for legacy compat backend takes TWO key types`
		`- # and we have to now deal with that ....`
		`- #`
		`- # Create ds_suffix here else it won't be in scope ....`
		`- ds_suffix = ''`
		`- if len(backends) > 0:`
		`- ds_suffix = normalizeDN(backends[0]['nsslapd-suffix'])`

		`# Create certdb in sysconfidir`
		`self.log.debug("ACTION: Creating certificate database is %s", slapd['cert_dir'])`

		`- # Create dse.ldif with a temporary root password.`
		`- # The template is in slapd['data_dir']/dirsrv/data/template-dse.ldif`
		`- # Variables are done with %KEY%.`
		`- # You could cheat and read it in, do a replace of % to { and } then use format?`
		`- self.log.debug("ACTION: Creating dse.ldif")`
		`- dse = ""`
		`- with open(os.path.join(slapd['data_dir'], 'dirsrv', 'data', 'template-dse.ldif')) as template_dse:`
		`- for line in template_dse.readlines():`
		`- dse += line.replace('%', '{', 1).replace('%', '}', 1)`
		`-`
		`- with open(os.path.join(slapd['config_dir'], 'dse.ldif'), 'w') as file_dse:`
		`- file_dse.write(dse.format(`
		`- schema_dir=slapd['schema_dir'],`
		`- lock_dir=slapd['lock_dir'],`
		`- tmp_dir=slapd['tmp_dir'],`
		`- cert_dir=slapd['cert_dir'],`
		`- ldif_dir=slapd['ldif_dir'],`
		`- bak_dir=slapd['backup_dir'],`
		`- run_dir=slapd['run_dir'],`
		`- inst_dir=slapd['inst_dir'],`
		`- log_dir=slapd['log_dir'],`
		`- fqdn=general['full_machine_name'],`
		`- ds_port=slapd['port'],`
		`- ds_user=slapd['user'],`
		`- rootdn=slapd['root_dn'],`
		`- # ds_passwd=slapd['root_password'],`
		`- ds_passwd=self._secure_password, # We set our own password here, so we can connect and mod.`
		`- ds_suffix=ds_suffix,`
		`- config_dir=slapd['config_dir'],`
		`- db_dir=slapd['db_dir'],`
		`- ))`
		`-`
		`- # open the connection to the instance.`
		`+ # BELOWE THIS LINE - all actions are now ONLINE changes to the directory server.`
		`+ # if it all possible, ALWAYS ADD NEW INSTALLER CHANGES AS ONLINE ACTIONS.`

		`# Should I move this import? I think this prevents some recursion`
		`from lib389 import DirSrv`

wrappers/systemd.template.service.in

file modified

+32 -6

		`@@ -23,22 +23,48 @@`
		`NotifyAccess=all`
		`TimeoutStartSec=0`
		`TimeoutStopSec=600`
		`- EnvironmentFile=@initconfigdir@/@package_name@`
		`- EnvironmentFile=@initconfigdir@/@package_name@-%i`
		`+ EnvironmentFile=-@initconfigdir@/@package_name@`
		`+ EnvironmentFile=-@initconfigdir@/@package_name@-%i`
		`PIDFile=@localstatedir@/run/@package_name@/slapd-%i.pid`
		`ExecStartPre=@libexecdir@/ds_systemd_ask_password_acl @instconfigdir@/slapd-%i/dse.ldif`
		`ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i @localstatedir@/run/@package_name@/slapd-%i.pid`

		`+ #### To change any of these values or directives, you should use a drop in file`
		`+ # such as: /etc/systemd/system/dirsrv@<instance>.d/custom.conf`
		`+`
		`+ # These are from man systemd.exec and man systemd.resource-control`
		`+`
		`+ # This controls the resources to the direct child of systemd, in`
		`+ # this case ns-slapd. Because we are type notify we recieve these`
		`+ # limits correctly.`
		`+`
		`+ # This controls the number of file handles avaliable. File handles`
		`+ # correlate to sockets for the process, and our access to logs and`
		`+ # databases.`
		`+ LimitNOFILE=16384`
		`+`
		`+ # You can limit the memory in the cgroup with these, and ns-slapd`
		`+ # will account for them in it's autotuning.`
		`+ # Memory account may be controlled by DefaultMemoryAccounting= in systemd-system.conf`
		`+ # MemoryAccounting=true`
		`+ # MemoryLimit=bytes`
		`+`
		`+ # Limits on the size of coredump that may be produced by the process. It's not`
		`+ # specified how this interacts with coredumpd.`
		`+ # 0 means not to produce cores.`
		`+ # This value is 64G`
		`+ LimitCORE=68719476736`
		`+`
		`+ # Limit number of processes (threads) we may spawn. We don't advise you change`
		`+ # this as DS will autodetect your threads / cpus and adjust as needed.`
		`+ # LimitNPROC=`
		`+`
		`# Hardening options:`
		`# PrivateDevices=true`
		`# ProtectSystem=true`
		`# ProtectHome=true`
		`# PrivateTmp=true`

		`- # if you need to set other directives e.g. LimitNOFILE=8192`
		`- # set them in this file`
		`- .include @initconfigdir@/@package_name@.systemd`
		`-`
		`[Install]`
		`WantedBy=multi-user.target`

wrappers/systemd.template.sysconfig

file removed

-29

		`@@ -1,29 +0,0 @@`
		`- [Service]`
		`- # These are from man systemd.exec and man systemd.resource-control`
		`-`
		`- # This controls the resources to the direct child of systemd, in`
		`- # this case ns-slapd. Because we are type notify we recieve these`
		`- # limits correctly.`
		`-`
		`- # This controls the number of file handles avaliable. File handles`
		`- # correlate to sockets for the process, and our access to logs and`
		`- # databases.`
		`- LimitNOFILE=16384`
		`-`
		`- # You can limit the memory in the cgroup with these, and ns-slapd`
		`- # will account for them in it's autotuning.`
		`- # Memory account may be controlled by DefaultMemoryAccounting= in systemd-system.conf`
		`- # MemoryAccounting=true`
		`- # MemoryLimit=bytes`
		`-`
		`- # Limits on the size of coredump that may be produced by the process. It's not`
		`- # specified how this interacts with coredumpd.`
		`- # 0 means not to produce cores.`
		`- # This value is 64G`
		`- LimitCORE=68719476736`
		`-`
		`- # Limit number of processes (threads) we may spawn. We don't advise you change`
		`- # this as DS will autodetect your threads / cpus and adjust as needed.`
		`- # LimitNPROC=`
		`-`
		`-`

wrappers/systemd.template.xsan.service.in

file modified

+30 -3

		`@@ -35,15 +35,42 @@`
		`ExecStartPre=@libexecdir@/ds_systemd_ask_password_acl @instconfigdir@/slapd-%i/dse.ldif`
		`ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i @localstatedir@/run/@package_name@/slapd-%i.pid`

		`+ #### To change any of these values or directives, you should use a drop in file`
		`+ # such as: /etc/systemd/system/dirsrv@<instance>.d/custom.conf`
		`+`
		`+ # These are from man systemd.exec and man systemd.resource-control`
		`+`
		`+ # This controls the resources to the direct child of systemd, in`
		`+ # this case ns-slapd. Because we are type notify we recieve these`
		`+ # limits correctly.`
		`+`
		`+ # This controls the number of file handles avaliable. File handles`
		`+ # correlate to sockets for the process, and our access to logs and`
		`+ # databases.`
		`+ LimitNOFILE=16384`
		`+`
		`+ # You can limit the memory in the cgroup with these, and ns-slapd`
		`+ # will account for them in it's autotuning.`
		`+ # Memory account may be controlled by DefaultMemoryAccounting= in systemd-system.conf`
		`+ # MemoryAccounting=true`
		`+ # MemoryLimit=bytes`
		`+`
		`+ # Limits on the size of coredump that may be produced by the process. It's not`
		`+ # specified how this interacts with coredumpd.`
		`+ # 0 means not to produce cores.`
		`+ # This value is 64G`
		`+ LimitCORE=68719476736`
		`+`
		`+ # Limit number of processes (threads) we may spawn. We don't advise you change`
		`+ # this as DS will autodetect your threads / cpus and adjust as needed.`
		`+ # LimitNPROC=`
		`+`
		`# Hardening options:`
		`# PrivateDevices=true`
		`# ProtectSystem=true`
		`# ProtectHome=true`
		`# PrivateTmp=true`

		`- # if you need to set other directives e.g. LimitNOFILE=8192`
		`- # set them in this file`
		`- .include @initconfigdir@/@package_name@.systemd`

		`[Install]`
		`WantedBy=multi-user.target`

firstyear commented 5 years ago

Bug Description: As sysconfig isn't cross platform compatible, and
there are some potential plans to remove it from our systemd files,
we need to make sure that lib389 can handle this file not being present
in new installs.

Fix Description: Thankfully, we have a file we can always guarantee
exists: dse.ldif. This makes /etc/dirsrv/slapd-instance the only
fixed location in the server now, all other locations can be "moved".

This patch:
* Fixes a large number of removal regressions
* Add comments and warnings throughout remove and setup to help
prevent future regresions
* Create no longer creates /etc/sysconfig/dirsrv-instance
* Create makes dse.ldif first as it's the marker location
* Remove works when there is no marker file (but will remove if it
exists)
* Listing now ignores /etc/sysconfig, and reads dse.ldif instead
with a follow up https://pagure.io/389-ds-base/issue/50207 to
parse data from this file for offline

https://pagure.io/389-ds-base/issue/50208

Author: William Brown william@blackhats.net.au

Review by: ???

spichugi commented 5 years ago

It fails on fedora 29...

Feb 11 05:17:19 host-8-246-147.host.centralci.eng.rdu2.redhat.com systemd[1]: /usr/lib/systemd/system/dirsrv@.service:16: .include directives are deprecated, and support for them will be removed in a future version of systemd. Please use drop-in files instead.
Feb 11 05:17:19 host-8-246-147.host.centralci.eng.rdu2.redhat.com systemd[1]: dirsrv@test_dscreate.service: Failed to load environment files: No such file or directory
Feb 11 05:17:19 host-8-246-147.host.centralci.eng.rdu2.redhat.com systemd[1]: dirsrv@test_dscreate.service: Failed to run 'start-pre' task: No such file or directory
Feb 11 05:17:19 host-8-246-147.host.centralci.eng.rdu2.redhat.com systemd[1]: dirsrv@test_dscreate.service: Failed with result 'resources'.

spichugi commented 5 years ago

Also, we probably should check if FreeIPA works correctly with the change.

firstyear commented 5 years ago

I wonder if that failure is an error in our systemd unit file?

firstyear commented 5 years ago

@spichugi IPA will fail as they write the KRB5_KTNAME to /etc/sysconfig/dirsrv-instance still, so they need to make changes on their end.

rebased onto 6a594c58cedae471bdd8f4927179df5d3619e067

5 years ago

firstyear commented 5 years ago

Okay, so this rebase fixes that, and also fixes some of the other warnings you are getting from systemd there.

abbra commented 5 years ago

My understanding is that we only will need to move KRB5_KTNAME definition to /etc/systemd/system/dirsrv@<INSTANCE>.service.d/ipa.conf so that it will automatically be loaded by systemd.

abbra commented 5 years ago

Or we can actually keep /etc/sysconfig/dirsrv* as it is and create /etc/systemd/system/dirsrv@INSTANCE.service.d/ipa-sysconfig.conf that has

EnvironmentFile=-/etc/sysconfig/dirsrv
EnvironmentFile=-/etc/sysconfig/dirsrv-%i

This way we keep the existing configuration at place and if they are missing, they'll get simply ignored. Any additional configuration options can be written directly to /etc/systemd/system/dirsrv@<INSTANCE>.service.d/ipa.conf

vashirov commented 5 years ago

FreeIPA ticket for tracking: https://pagure.io/freeipa/issue/7860

@firstyear, let's coordinate this change with FreeIPA to avoid an unnecessary breakage.

firstyear commented 5 years ago

@abbra That would work. But I'm not removing the EnvironmentFile lines - I'm just not creating /etc/sysconfig/dirsrv-* by default anymore. So your env file solution already is there in the change?

So a better idea is you create the ipa-sysconfig.conf that contains "Environment=KRB5_KTNAME=..." I think. Alternately, you could do something like /etc/ipa/ds.conf and then have that as the EnvironmentFile=- that we include.

Either way, I think this is pretty easy for IPA to solve. Nothing in this directly breaks existing IPA installs, only new ones when they are created, and only because they probably expect /etc/sysconfig/dirsrv-instance to be there - if IPA recreates that file of their own, that's not going to bother me, and should keep working.

spichugi commented 5 years ago

The build fails on F29 now...

BUILDSTDERR: make[1]: *** No rule to make target 'wrappers/dirsrv.systemd', needed by 'all-am'.  Stop.
make[1]: Leaving directory '/builddir/build/BUILD/389-ds-base-1.4.1.1.20190212git0c3045cf1'
BUILDSTDERR: make: *** [Makefile:4451: all] Error 2

firstyear commented 5 years ago

I must have missed something in the make file then :) I'll check this out (I don't have a systemd capable build system and haven't used one in a long time ...)

rebased onto 08cf40f4896bb23ebeae277c207dbc58daaad219

5 years ago

firstyear commented 5 years ago

Okay, this should fix the issue you are seeing. I think that maybe the next step is open an issue on freeipa (blocking?) about the env file needing to be created by them, but otherwise, I think we can't do much more here. Just need to make sure they are good to go.

firstyear commented 5 years ago

https://pagure.io/freeipa/issue/7860 was created by @vashirov, and I've commented on it to help make the changes required clear.

abbra commented 5 years ago

@firstyear, yes I have no problem with this move -- we have all the code and example how to do that as we did the move for httpd.service some time ago to use the same scheme. As long as we coordinate a release time to Fedora 30 repositories, I'm fine.

spichugi commented 5 years ago

BUILDSTDERR: error: File not found: /builddir/build/BUILDROOT/389-ds-base-1.4.1.1-20190213git08cf40f48.fc29.x86_64/etc/sysconfig/dirsrv

I think it really makes sense for you to have some environment for testing Fedora part...

Edited 5 years ago by spichugi

rebased onto 1eb753938c05c0aad6cd329f6ca14857621ca024

5 years ago

firstyear commented 5 years ago

@abbra Great, in that case, I'll leave it to you in the IPA ticket that has been opened to finish up that side. I'm sure we'll commit this once I finish fixing my mistakes :)

@spichugi I think this goes to a point where, yes, most of the developers of this project are on Fedora/RH. That was fine for a number of years, but it was exclusionary to debian/SUSE and others. Linux is not Fedora centric. This project is now getting broader attention, and to a point where SUSE is now financially investing in me to develop it as their LDAP platform for the future.

As a result it is not feasible for everyone to install every permutation and combination of OS to test on. We are an upstream project, and we need to do things in that light. And the downstream integrators to distributions need to also be mindful in reviews and testing for integration challenges. It is not logistically possible to just ask someone "ohh hey, do you mind installing a whole new OS ...". A response like that says our upstream is deeply tied into a specific vendor - that's not sustainable at all.

We need to step away from the RH/Fedora only mindset, and start to think about broader development and deployment scenarios. Diversity will promote health. For example I am probably the only person developing on SUSE, so I am finding those problems. I'm also likely the only person developing on and integrating in containers so I am finding those issues. This makes the project more robust and solid. I don't ask you to test on SUSE - that's what I'm paid to do.

My goal is 389 on SUSE and in containers. As an upstream our collective input is what will make 389 work best on a variety of platforms.

Now, I think the error you have pasted is from the rpm build - I think I have corrected it in the last commit.

abbra commented 5 years ago

It would, perhaps, help to set a simple CI test to build packages. Pagure supports Fedora CI integration so a test can be run easily with a container of any choice. I might look into adding this test interface support next week -- purely to allow building packages as a test of a pull request

abbra commented 5 years ago

An alternative would be for @firstyear to set up a space in OBS and let us trigger builds there for any pull request. OBS can build for Fedora and openSUSE already.

tbordaz commented 5 years ago

I agree with @firstyear that 389-ds should not be Fedora centric and it would be a constraint to ask on each PR to run build/test on many distrib/plateform. @abbra is right, PR-CI is a good answer to this. Now I am not a PR-CI expert, @vashirov do you know our status on that side and how much effort it would be to make PR-CI basic tests (build/install/basic) on several distrib/plateform ?

spichugi commented 5 years ago

The last commit fixed the issue. And the code looks good to me. We can proceed here (according to the agreement with FreeIPA).

@spichugi I think this goes to a point where, yes, most of the developers of this project are on Fedora/RH. That was fine for a number of years, but it was exclusionary to debian/SUSE and others. Linux is not Fedora centric. This project is now getting broader attention, and to a point where SUSE is now financially investing in me to develop it as their LDAP platform for the future.

Sure. For now, we don't have the PR CI and if you'd have the Fedora environment - it will speed up the process (our time zones are too different and it takes to much time). That was my point.

If we'll have the PR CI then the issue will be solved.

firstyear commented 5 years ago

Sounds good. I'll merge this shortly then. Thanks,

rebased onto e373f39

5 years ago

Pull-Request has been merged by firstyear

5 years ago

mreynolds commented 5 years ago

@firstyear - this breaks "dsctl remove". I can no longer remove instances after this PR was merged. Please fix ASAP or we will have to revert it. Thanks

mreynolds commented 5 years ago

Here is example showing the issue:

[root@localhost memberof_plugin]# ls /etc/dirsrv/
config slapd-consumer1 slapd-localhost slapd-master1 slapd-standalone1 ssca
schema slapd-hub1 slapd-stadalone1

[root@localhost memberof_plugin]# dsctl consumer1 remove --do-it
No such instance consumer1
[root@localhost memberof_plugin]# dsctl dirsrv-consumer1 remove --do-it
No such instance dirsrv-consumer1
[root@localhost memberof_plugin]# dsctl slapd-consumer1 remove --do-it
No such instance slapd-consumer1
[root@localhost memberof_plugin]# remove-ds.pl -i slapd-consumer1
Instance slapd-consumer1 removed.

So the old perl tools still work, but dsctl remove is now broken on fedora(and RHEL)

mreynolds commented 5 years ago

@firstyear I know this was asked of you before, but can you please test your patches on a Fedora VM before you summit them? It will save us all a lot of time and frustration. Thanks for your cooperation!!!

firstyear commented 5 years ago

@mreynolds I'll investigate this today. I think revert is the wrong answer though. In general I'd like to say there is code merged by everyone else here that has broken docker, suse, and without-systemd support multiple times. I don't revert every issue I find, I fix the issues that exist in front of me (or at least investigate enough cause to work out what's going on). Revert is a heavy handed tactic, and I don't approve of it.

I don't have the resources timewise to test on Fedora, just the same way that you probably don't have the time to test on docker, opensuse and suse leap. I think part of being a community is always looking after different aspects, and just picking up as we go.

mreynolds commented 5 years ago

@mreynolds I'll investigate this today. I think revert is the wrong answer though. In general I'd like to say there is code merged by everyone else here that has broken docker, suse, and without-systemd support multiple times. I don't revert every issue I find, I fix the issues that exist in front of me (or at least investigate enough cause to work out what's going on). Revert is a heavy handed tactic, and I don't approve of it.
I don't have the resources timewise to test on Fedora, just the same way that you probably don't have the time to test on docker, opensuse and suse leap. I think part of being a community is always looking after different aspects, and just picking up as we go.

Revert is the last resort of course, but if this doesn't get fixed soon there will not be an option as there are already upstream builds done that now have this issue. I am being pressured to fix this asap...

Sorry to hear you are unwilling to install a fedora VM, it just means we will now have to spend our time fully testing your patches before we can ack them. We are hoping to get this automatically running in pagure, but for now it is not, and it needs to be run manually...

firstyear commented 5 years ago

https://pagure.io/389-ds-base/pull-request/50242
This resolves the issue.

I think that this is an over-reaction. As mentioned, I have to fix mistakes left by everyone else, quite frequently. I'm not asking you to "install suse" and every permutation of linux. I fix the issues I see, and I talk to people when they arise. I think that there are probably better solutions to this problem than saying you'll have to "check all my work" - which probably is just going to consume more of your time, than do good. Everyone in the team is human, everyone makes mistakes, we just deal with them as they come up.

mreynolds commented 5 years ago

The change to the spec file broke FreeIPA upgrades from Fedoras 29 to Fedora 30. Fresh install on F30 work fine, but upgrades fail, and this is now being marked as a blocker.

Looks like this was merged before IPA was ready to make the change (@abbra)

So I need to revert this change in the Fedora upstream spec file until FreeIPA has a chance to make the their changes

mreynolds commented 5 years ago

The change to the spec file broke FreeIPA upgrades from Fedoras 29 to Fedora 30. Fresh install on F30 work fine, but upgrades fail, and this is now being marked as a blocker.
Looks like this was merged before IPA was ready to make the change (@abbra)
So I need to revert this change in the Fedora upstream spec file until FreeIPA has a chance to make the their changes

FreeIPA merged their change and did a build so this is all good now!

firstyear commented 5 years ago

Yay! Thanks!

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3272

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.