#50034 Ticket 50022, 50012, 49956, and 49800: Various dsctl/dscreate fixes
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket50012  into  master

file modified
+1 -1
@@ -33,7 +33,7 @@ 

  

  # Create the example setup inf. It's valid for containers!

  # Build the instance from the new installer tools.

- RUN /usr/sbin/dscreate create-template > /root/ds-setup.inf && /usr/sbin/dscreate -v fromfile /root/ds-setup.inf --containerised

+ RUN /usr/sbin/dscreate create-template > /root/ds-setup.inf && /usr/sbin/dscreate -v from-file /root/ds-setup.inf --containerised

  

  # Finally add the volumes, they will inherit the contents of these directories.

  VOLUME /etc/dirsrv

@@ -1405,7 +1405,7 @@ 

      $("#remove-server-btn").on("click", function () {

        popup_confirm("Are you sure you want to this remove instance: <b>" + server_id + "</b>", "Confirmation", function (yes) {

          if (yes) {

-           var cmd = [DSCTL, server_id, "remove", "--doit"];

+           var cmd = [DSCTL, server_id, "remove", "--do-it"];

            $("#ds-remove-inst").html("<span class=\"spinner spinner-xs spinner-inline\"></span> Removing instance <b>" + server_id + "</b>...");

            $("#remove-instance-form").modal('toggle');

            log_cmd('#remove-server-btn (click)', 'Remove instance', cmd);
@@ -1581,7 +1581,7 @@ 

                /*

                 * Next, create the instance...

                 */

-               cmd = [DSCREATE, 'fromfile', setup_file];

+               cmd = [DSCREATE, 'from-file', setup_file];

                cockpit.spawn(cmd, { superuser: true, "err": "message", "environ": [ENV] }).fail(function(ex) {

                  // Failed to create the new instance!

                  cockpit.spawn(rm_cmd, { superuser: true });  // Remove Inf file with clear text password

file modified
+1 -1
@@ -24,7 +24,7 @@ 

                      action='store_true', default=False, dest='verbose')

  subparsers = parser.add_subparsers(help="action")

  

- fromfile_parser = subparsers.add_parser('fromfile', help="Create an instance of Directory Server from an inf answer file")

+ fromfile_parser = subparsers.add_parser('from-file', help="Create an instance of Directory Server from an inf answer file")

  fromfile_parser.add_argument('file', help="Inf file to use with prepared answers. You can generate an example of this with 'dscreate create-template'")

  fromfile_parser.add_argument('-n', '--dryrun', help="Validate system and configurations only. Do not alter the system.",

                               action='store_true', default=False)

file modified
+25 -2
@@ -14,24 +14,35 @@ 

  import logging

  import sys

  import signal

+ from lib389.utils import get_instance_list

  from lib389.cli_base import _get_arg

  from lib389 import DirSrv

  from lib389.cli_ctl import instance as cli_instance

  from lib389.cli_ctl import dbtasks as cli_dbtasks

  from lib389.cli_base import disconnect_instance, setup_script_logger

+ from lib389.cli_ctl.instance import instance_remove_all

  

  parser = argparse.ArgumentParser()

  parser.add_argument('-v', '--verbose',

          help="Display verbose operation tracing during command execution",

          action='store_true', default=False

      )

- parser.add_argument('instance',

+ parser.add_argument('instance', nargs='?', default=False,

          help="The name of the instance to act upon",

      )

  parser.add_argument('-j', '--json',

          help="Return result in JSON object",

          default=False, action='store_true'

      )

+ parser.add_argument('-l', '--list',

+         help="List available Directory Server instances",

+         default=False, action='store_true'

+     )

+ 

+ parser.add_argument('--remove-all', nargs="?", default=False, const=None,

+         help="Remove all instances of Directory Server (you can also provide an optional directory prefix for this argument)",

+     )

+ 

  subparsers = parser.add_subparsers(help="action")

  cli_instance.create_parser(subparsers)

  cli_dbtasks.create_parser(subparsers)
@@ -56,6 +67,19 @@ 

  

      log.debug("Called with: %s", args)

  

+     if args.list:

+         insts = get_instance_list()

+         for inst in insts:

+             print(inst)

+         sys.exit(0)

+     elif args.remove_all is not False:

+         instance_remove_all(log, args)

+         sys.exit(0)

+     elif not args.instance:

+         log.error("error: the following arguments are required: instance")

+         parser.print_help()

+         sys.exit(1)

+ 

      # Assert we have a resources to work on.

      if not hasattr(args, 'func'):

          log.error("No action provided, here is some --help.")
@@ -63,7 +87,6 @@ 

          sys.exit(1)

  

      # Connect

-     # inst = None

      inst = DirSrv(verbose=args.verbose)

  

      result = True

@@ -1192,7 +1192,7 @@ 

  

              @return None

  

-             @raise None

+             @raise ValueError

          '''

  

          if self.status() is True:
@@ -1230,7 +1230,7 @@ 

                  time.sleep(1)

                  pid = pid_from_file(self.ds_paths.pid_file)

              if pid == 0 or pid is None:

-                 raise ValueError

+                 raise ValueError('Failed to start DS')

              # Wait

              while not pid_exists(pid) and count > 0:

                  # It looks like DS changes the value in here at some point ...
@@ -1240,7 +1240,7 @@ 

                  time.sleep(1)

                  count -= 1

              if not pid_exists(pid):

-                 raise Exception("Failed to start DS")

+                 raise ValueError("Failed to start DS")

          if post_open:

              self.open()

  
@@ -1254,7 +1254,7 @@ 

  

              @return None

  

-             @raise None

+             @raise ValueError

          '''

          if self.status() is False:

              return
@@ -1270,7 +1270,7 @@ 

              count = timeout

              pid = pid_from_file(self.ds_paths.pid_file)

              if pid == 0 or pid is None:

-                 raise ValueError

+                 raise ValueError("Failed to stop DS")

              os.kill(pid, signal.SIGTERM)

              # Wait

              while pid_exists(pid) and count > 0:
@@ -1328,7 +1328,7 @@ 

  

              @return None

  

-             @raise None

+             @raise ValueError

          '''

          self.stop(timeout)

          time.sleep(1)

@@ -8,8 +8,10 @@ 

  

  from lib389._constants import *

  

+ from lib389 import DirSrv

  from lib389.tools import DirSrvTools

  from lib389.instance.setup import SetupDs

+ from lib389.utils import get_instance_list

  from lib389.instance.remove import remove_ds_instance

  from getpass import getpass

  import os
@@ -35,21 +37,24 @@ 

  

  def instance_restart(inst, log, args):

      inst.restart(post_open=False)

+     log.info('Instance "{}" has been restarted'.format(inst.serverid))

  

  

  def instance_start(inst, log, args):

      inst.start(post_open=False)

+     log.info('Instance "{}" has been started'.format(inst.serverid))

  

  

  def instance_stop(inst, log, args):

      inst.stop()

+     log.info('Instance "{}" has been stopped'.format(inst.serverid))

  

  

  def instance_status(inst, log, args):

      if inst.status() is True:

-         log.info("Instance is running")

+         log.info('Instance "{}" is running'.format(inst.serverid))

      else:

-         log.info("Instance is not running")

+         log.info('Instance "{}" is not running'.format(inst.serverid))

  

  

  def instance_create_interactive(inst, log, args):
@@ -124,25 +129,54 @@ 

      return True

  

  

+ def instance_remove_all(log, args):

+     """Remove all instances - clean sweep!

+     """

+ 

+     inst_names = get_instance_list(args.remove_all)

+     if len(inst_names) > 0:

+         answer = input("Are you sure you want to remove all the Directory Server instances? (Yes/no): ")

+         if answer != 'Yes':

+             print("Aborted removal of all instances")

+             return

+ 

+         # Do it!

+         list_inst = DirSrv(verbose=args.verbose)

+         insts = list_inst.list(all=True, serverid=inst_names[0])

+         for inst in insts:

+             remove_inst = DirSrv(verbose=args.verbose)

+             remove_inst.allocate(inst)

+             try:

+                 log.info("Removing instance: slapd-" + str(remove_inst.serverid))

+                 remove_ds_instance(remove_inst)

+             except Exception as e:

+                 log.fatal('Failed to remove all instances: ' + str(e))

+                 sys.exit(1)

+         log.info('All instances have been successfully removed')

+     else:

+         print("No instances to remove")

+ 

+ 

  def instance_remove(inst, log, args):

      if not args.ack:

-         log.info("""Not removing: if you are sure, add --doit""")

+         # Some day do some type of dry-run validation?

+         log.info("""Not removing: if you are sure, add --do-it""")

          return True

      else:

          log.info("""

- About to remove instance %s!

+ About to remove instance (%s)!

  If this is not what you want, press ctrl-c now ...

          """ % inst.serverid)

-     for i in range(1, 6):

-         log.info('%s ...' % (5 - int(i)))

-         time.sleep(1)

-     log.info('Removing instance ...')

-     try:

-         remove_ds_instance(inst)

-         log.info('Completed instance removal')

-     except:

-         log.fatal('Instance removal failed')

-         return False

+         for i in range(1, 6):

+             log.info('%s ...' % (6 - int(i)))

+             time.sleep(1)

+         log.info('Removing instance ...')

+         try:

+             remove_ds_instance(inst)

+             log.info('Completed instance removal')

+         except:

+             log.fatal('Instance removal failed')

+             return False

  

  

  def create_parser(subcommands):
@@ -163,8 +197,9 @@ 

      status_parser.set_defaults(func=instance_status)

  

      remove_parser = subcommands.add_parser('remove', help="Destroy an instance of Directory Server, and remove all data.")

-     remove_parser.add_argument('--doit', dest="ack", help="By default we do a dry run. This actually initiates the removal.",

-                                action='store_true', default=False)

      remove_parser.set_defaults(func=instance_remove)

+     remove_parser.add_argument('--do-it', dest="ack", help="By default we do a dry run. This actually initiates the removal of the instance.",

+                                action='store_true', default=False)

+ 

  

  

@@ -86,6 +86,7 @@ 

  

  SECTION = 'slapd'

  

+ 

  class Paths(object):

      def __init__(self, serverid=None, instance=None):

          """

file modified
+4 -4
@@ -811,9 +811,9 @@ 

          """

          try:

              grp.getgrnam(group)

-             print("OK group %s exists" % group)

+             log.debug("OK group %s exists" % group)

          except KeyError:

-             print("Adding group %s" % group)

+             log.debug("Adding group %s" % group)

              cmd = [GROUPADD, '-r', group]

              subprocess.Popen(cmd)

  
@@ -824,9 +824,9 @@ 

          """

          try:

              pwd.getpwnam(user)

-             print("OK user %s exists" % user)

+             log.debug("OK user %s exists" % user)

          except KeyError:

-             print("Adding user %s" % user)

+             log.debug("Adding user %s" % user)

              cmd = [USERADD, '-g', group, '-c', DEFAULT_USER_COMMENT, '-r',

                     '-d', home, '-s', NOLOGIN, user]

              subprocess.Popen(cmd)

@@ -1110,3 +1110,19 @@ 

      else:

          # Use LDAP

          return ("ldap://{}:{}".format(host, port), None)

+ 

+ 

+ def get_instance_list(prefix=None):

+     # List all server instances

+     conf_dir = (prefix or "") + "/etc/dirsrv/"

+     insts = []

+     try:

+         for inst in os.listdir(conf_dir):

+             if inst.startswith('slapd-') and not inst.endswith('.removed'):

+                 insts.append(inst)

+     except OSError as e:

+         log.error("Failed to check directory: {} - {}".format(conf_dir, str(e)))

+     insts.sort()

+     return insts

+ 

+ 

Description:

Fix 50022 - Confusing command line switches for dscreate and dsctl
Fix 50012 - Add option to dsctl to remove all instances
Fix 49956 - dsctl: add an option to list all available instances
Fix 49800 - Debug messages "OK user/group dirsrv exists" are emitted when lib389 cli tools are used

https://pagure.io/389-ds-base/issue/50022
https://pagure.io/389-ds-base/issue/50012
https://pagure.io/389-ds-base/issue/49956
https://pagure.io/389-ds-base/issue/49800

Reviewed by: ?

Let's use 2 words separated by dash here too :)

Perhaps this should be log.error of log.fatal?

You may want to ask for something more than "Y" here. Like "are you sure?" "yes I'm sure". Copy-paste is a good way to check. Github does this by making you type the repo name before you are allowed to delete it.

I agree with @vashirov, this should be log.error

Maybe just do conf_dir = (prefix or "") + "/etc/dirsrv/".

In the usage, we have -
remove - Destroy an instance of Directory Server, and remove all data.

But in actual it takes "--doit" to do it -
[root@172 upstream]# dsctl server remove
Not removing: if you are sure, add --do-it

Whereas for the "--removeall" option, it asks like this -
Are you sure you want to remove all the Directory Server instances? (y/n): y

IMO, Some consistency would be better here.
I suggest to have - Are you sure you want to remove all the Directory Server instances? (yes/no):
for both the options for consistency.
Thanks.

In the usage, we have -
remove - Destroy an instance of Directory Server, and remove all data.
But in actual it takes "--doit" to do it -
[root@172 upstream]# dsctl server remove
Not removing: if you are sure, add --do-it
Whereas for the "--removeall" option, it asks like this -
Are you sure you want to remove all the Directory Server instances? (y/n): y
IMO, Some consistency would be better here.
I suggest to have - Are you sure you want to remove all the Directory Server instances? (yes/no):
for both the options for consistency.

This is actually being discussed in https://pagure.io/389-ds-base/issue/50022

Since no one was in agreement I did not change "dsctl remove". As for not using "--do-it" with "--remove-all" that is because from the usage (arg parsing) perspective to have "--remote-all" and "--do-it" does not make sense. - so prompting for confirmation made more sense. We should probably do it "dsctl remove", but like I said there is disagreement over that at the moment.

rebased onto c4ebc4c5db9ddd92d1a782b1c86ab02efd3f91fd

5 years ago

New changes applied and rebased, please review...

rebased onto 5eb3b78d75ddd51323f3f4eaf0e505c36ba4e025

5 years ago

rebased onto bdfa1fbaedd0796d226d3d1352616cadc7bbba73

5 years ago

If you create an instance with a custom port (like 38901) it will complain on the removal:

Removed /etc/systemd/system/multi-user.target.wants/dirsrv@localhost1.service.
ValueError: Port tcp/38901 is not defined
ValueError: Port tcp/38901 is not defined
ValueError: Port tcp/38901 is not defined
CRITICAL: Failed to remove all instances: Failed to mangle port label: Command '['semanage', 'port', '-d', '-t', 'ldap_port_t', '-p', 'tcp', '38901']' returned non-zero exit status 1.

But the removal still happens.

Another thing, the UI doesn't count the changes and fails while I try to create an instance.

The rest looks good to me! Thanks!

If you create an instance with a custom port (like 38901) it will complain on the removal:
Removed /etc/systemd/system/multi-user.target.wants/dirsrv@localhost1.service.
ValueError: Port tcp/38901 is not defined
ValueError: Port tcp/38901 is not defined
ValueError: Port tcp/38901 is not defined
CRITICAL: Failed to remove all instances: Failed to mangle port label: Command '['semanage', 'port', '-d', '-t', 'ldap_port_t', '-p', 'tcp', '38901']' returned non-zero exit status 1.

But the removal still happens.

This is being addressed in this PR: https://pagure.io/389-ds-base/pull-request/50045

rebased onto 054ccc9d180ce7933c55aad6b847b5b63bf3779a

5 years ago

Fixed the other issue in UI for "fromfile" to "from-file".

rebased onto aec5233a6e7daaeaeb1437c122e3b92172cc4cf8

5 years ago

Sorry, I missed one more issue in the UI - '--doit' -> '--do-it'

The rest looks good to me! Ack

rebased onto 4fd73c5

5 years ago

Pull-Request has been merged by mreynolds

5 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/3093

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.

Pull-Request has been closed by spichugi

3 years ago