Refactor the Datastore Manager

There is a large amount of boiler-plate and/or copy-and-paste code in each datastore manager. As more managers are added, the time involved in maintaining all this code will continue to grow. To alleviate this and the bugs it promotes, a base manager class needs to be created where all common code can reside.

Launchpad Blueprint: https://blueprints.launchpad.net/trove/+spec/datastore-manager-refactor

Problem Description

When bugs are discovered and fixed in the manager code of one datastore, these changes are seldom transferred to all the other datastores. This causes a ‘drift’ in the stability of each experimental datastore as currently there are no third-party CI’s. In addition, when ‘new improved’ ways to solve a problem are implemented they are done differently across each manager, if at all. It is also difficult to transfer features (and coding knowledge) from one datastore to another, and the problem is exacerbated as code is copied-and-pasted when a new datastore manager is implemented.

A current example of this problem is the issue where instances flip from BUILD to ACTIVE and back again [1], or from BUILD->SHUTDOWN->ACTIVE. [2] Fixing this means changing how prepare works, and currently this change would be required to be made in each datastore manager. Implementing this in a uniform way would be almost impossible with the current architecture of the manager code base.

Proposed Change

A new ‘Manager’ class will be created that will be the base class for all datastore managers. In order to keep the scope reasonable, only the barest minimum of functionality will be pulled back into the base class in the initial implementation - this will include methods that are currently ‘boiler-plate’ code (such as the response to the rpc_ping and get_fileststem_stats calls, and the periodic task, update_status).

A mechanism for encapsulating functionality will also be needed in order for the base manager to be able to execute datastore-specific instructions. This will be accomplished through the use of properties that can be overridden by each descendant. Some required ones (such as the ‘status’ object) will be declared abstract and must be present; others (such as the new configuration manager, and potentially a future dictionary of strategies) will be optional.

The new Manager class will reside in the datastore module:

trove/guestagent/datastore/manager.py

This is alongside the existing service.py file (which contains the existing BaseDbStatus class). The directory structure of the MySQL derived classes will also be tidied a bit and will end up looking like the following:

trove
+-- guestagent
    +-- datastore
        +-- __init__.py
        +-- manager.py          <-- new 'base' manager
        +-- service.py
        +-- experimental
         +-- __init__.py
         +-- cassandra
          +-- __init__.py
          +-- manager.py
          +-- service.py
          +-- system.py

        <other experimental datastore modules>

         +-- redis
          +-- __init__.py
          +-- manager.py
          +-- service.py
          +-- system.py
         +-- vertica
             +-- __init__.py
             +-- manager.py
             +-- service.py
             +-- system.py
        +-- mysql_common        <-- new module
         +-- __init__.py
         +-- manager.py      <-- renamed from mysql/manager_base.py
         +-- service.py      <-- renamed from mysql/service_base.py
        +-- mysql
         +-- __init__.py
         +-- manager.py
         +-- service.py
        +-- technical-preview
            +-- __init__.py

Within the context of this refactor (as a proof-of-concept), the issue with having instances flip in-and-out of BUILD state will be addressed properly. The prepare method will be moved into the base class, which will seamlessly implement the code required to ensure that no notifications are sent until it is guaranteed that prepare has finished successfully. The existing prepare methods will be renamed ‘do_prepare’ and will be called from within the base prepare method.

This manner of determining whether prepare has finished will be accomplished by having the manager write a file at the start and at the successful conclusion of the prepare operation. Any errors (exceptions) will be trapped and logged and the instance set into the FAILED state.

A sample of what this will look like in the base manager is as follows, although additional properties may be defined as they are deemed necessary (and others can be added in future cleanup work):

class Manager(periodic_task.PeriodicTasks):
    """This is the base class for all datastore managers.  Over time, common
    functionality should be pulled back here from the existing managers.
    """

    def __init__(self, manager_name):

        super(Manager, self).__init__(CONF)

        # Manager properties
        self.__manager_name = manager_name
        self.__manager = None
        self.__prepare_error = False

    @property
    def manager_name(self):
        """This returns the passed-in name of the manager."""
        return self.__manager_name

    @property
    def manager(self):
        """This returns the name of the manager."""
        if not self.__manager:
            self.__manager = CONF.datastore_manager or self.__manager_name
        return self.__manager

    @property
    def prepare_error(self):
        return self.__prepare_error

    @prepare_error.setter
    def prepare_error(self, prepare_error):
        self.__prepare_error = prepare_error

    @property
    def configuration_manager(self):
        """If the datastore supports the new-style configuration manager,
        it should override this to return it.
        """
        return None

    @abc.abstractproperty
    def status(self):
        """This should return an instance of a status class that has been
        inherited from datastore.service.BaseDbStatus.  Each datastore
        must implement this property.
        """
        return None

    ################
    # Status related
    ################
    @periodic_task.periodic_task
    def update_status(self, context):
        """Update the status of the trove instance. It is decorated with
        perodic_task so it is called automatically.
        """
        LOG.debug("Update status called.")
        self.status.update()

    def rpc_ping(self, context):
        LOG.debug("Responding to RPC ping.")
        return True

    #################
    # Prepare related
    #################
    def prepare(self, context, packages, databases, memory_mb, users,
                device_path=None, mount_point=None, backup_info=None,
                config_contents=None, root_password=None, overrides=None,
                cluster_config=None, snapshot=None):
        """Set up datastore on a Guest Instance."""
        LOG.info(_("Starting datastore prepare for '%s'.") % self.manager)
        self.status.begin_install()
        post_processing = True if cluster_config else False
        try:
            self.do_prepare(context, packages, databases, memory_mb,
                            users, device_path, mount_point, backup_info,
                            config_contents, root_password, overrides,
                            cluster_config, snapshot)
        except Exception as ex:
            self.prepare_error = True
            LOG.exception(_("An error occurred preparing datastore: %s") %
                          ex.message)
            raise
        finally:
            LOG.info(_("Ending datastore prepare for '%s'.") % self.manager)
            self.status.end_install(error_occurred=self.prepare_error,
                                    post_processing=post_processing)
        # At this point critical 'prepare' work is done and the instance
        # is now in the correct 'ACTIVE' 'INSTANCE_READY' or 'ERROR' state.
        # Of cource if an error has occurred, none of the code that follows
        # will run.
        LOG.info(_('Completed setup of datastore successfully.'))

        # We only create databases and users automatically for non-cluster
        # instances.
        if not cluster_config:
            try:
                if databases:
                    LOG.debug('Calling add databases.')
                    self.create_database(context, databases)
                if users:
                    LOG.debug('Calling add users.')
                    self.create_user(context, users)
            except Exception as ex:
                LOG.exception(_("An error occurred creating databases/users: "
                                "%s") % ex.message)
                raise

        try:
            LOG.debug('Calling post_prepare.')
            self.post_prepare(context, packages, databases, memory_mb,
                              users, device_path, mount_point, backup_info,
                              config_contents, root_password, overrides,
                              cluster_config, snapshot)
        except Exception as ex:
            LOG.exception(_("An error occurred in post prepare: %s") %
                          ex.message)
            raise

    @abc.abstractmethod
    def do_prepare(self, context, packages, databases, memory_mb, users,
                   device_path, mount_point, backup_info, config_contents,
                   root_password, overrides, cluster_config, snapshot):
        """This is called from prepare when the Trove instance first comes
        online.  'Prepare' is the first rpc message passed from the
        task manager.  do_prepare handles all the base configuration of
        the instance and is where the actual work is done.  Once this method
        completes, the datastore is considered either 'ready' for use (or
        for final connections to other datastores) or in an 'error' state,
        and the status is changed accordingly.  Each datastore must
        implement this method.
        """
        pass

    def post_prepare(self, context, packages, databases, memory_mb, users,
                     device_path, mount_point, backup_info, config_contents,
                     root_password, overrides, cluster_config, snapshot):
        """This is called after prepare has completed successfully.
        Processing done here should be limited to things that will not
        affect the actual 'running' status of the datastore (for example,
        creating databases and users, although these are now handled
        automatically).  Any exceptions are caught, logged and rethrown,
        however no status changes are made and the end-user will not be
        informed of the error.
        """
        LOG.debug('No post_prepare work has been defined.')
        pass

    #####################
    # File System related
    #####################
    def get_filesystem_stats(self, context, fs_path):
        """Gets the filesystem stats for the path given."""
        # TODO(peterstac) - note that fs_path is not used in this method.
        mount_point = CONF.get(self.manager).mount_point
        LOG.debug("Getting file system stats for '%s'" % mount_point)
        return dbaas.get_filesystem_volume_stats(mount_point)

    #################
    # Cluster related
    #################
    def cluster_complete(self, context):
        LOG.debug("Cluster creation complete, starting status checks.")
        self.status.end_install()

The base service class will be enhanced to contain the necessary methods to set a flag denoting whether prepare has finished of not. This will look something like the following (only new code is shown):

class BaseDbStatus(object):

    GUESTAGENT_DIR = '~'
    PREPARE_START_FILENAME = '.guestagent.prepare.start'
    PREPARE_END_FILENAME = '.guestagent.prepare.end'

    def __init__(self):
        self._prepare_completed = None

    @property
    def prepare_completed(self):
        if self._prepare_completed is None:
            # Force the file check
            self.prepare_completed = None
        return self._prepare_completed

    @prepare_completed.setter
    def prepare_completed(self, value):
        # Set the value based on the existence of the file; 'value' is
        # ignored
        # This is required as the value of prepare_completed is cached,
        # so this must be referenced any time the existence of the
        # file changes
        self._prepare_completed = os.path.isfile(
            guestagent_utils.build_file_path(
                self.GUESTAGENT_DIR, self.PREPARE_END_FILENAME))

    def begin_install(self):
        """Called right before DB is prepared."""
        prepare_start_file = guestagent_utils.build_file_path(
            self.GUESTAGENT_DIR, self.PREPARE_START_FILENAME)
        operating_system.write_file(prepare_start_file, '')
        self.prepare_completed = False

        self.set_status(instance.ServiceStatuses.BUILDING, True)

    def end_install(self, error_occurred=False, post_processing=False):
        """Called after prepare completes."""

        # Set the "we're done" flag if there's no error and
        # no post_processing is necessary
        if not (error_occurred or post_processing):
            prepare_end_file = guestagent_utils.build_file_path(
                self.GUESTAGENT_DIR, self.PREPARE_END_FILENAME)
            operating_system.write_file(prepare_end_file, '')
            self.prepare_completed = True

        final_status = None
        if error_occurred:
            final_status = instance.ServiceStatuses.FAILED
        elif post_processing:
            final_status = instance.ServiceStatuses.INSTANCE_READY

        if final_status:
            LOG.info(_("Set final status to %s.") % final_status)
            self.set_status(final_status, force=True)
        else:
            self._end_install_or_restart(True)

    def end_restart(self):
        self.restart_mode = False
        LOG.info(_("Ending restart."))
        self._end_install_or_restart(False)

    def _end_install_or_restart(self, force):
        """Called after DB is installed or restarted.
        Updates the database with the actual DB server status.
        """
        real_status = self._get_actual_db_status()
        LOG.info(_("Current database status is '%s'.") % real_status)
        self.set_status(real_status, force=force)

    @property
    def is_installed(self):
        """This is for compatibility - it may be removed during further
        cleanup.
        """
        return self.prepare_completed

    def set_status(self, status, force=False):
        """Use conductor to update the DB app status."""

        if force or self.is_installed:
            LOG.debug("Casting set_status message to conductor "
                      "(status is '%s')." % status.description)
            context = trove_context.TroveContext()

            heartbeat = {'service_status': status.description}
            conductor_api.API(context).heartbeat(
                CONF.guest_id, heartbeat, sent=timeutils.float_utcnow())
            LOG.debug("Successfully cast set_status.")
            self.status = status
        else:
            LOG.debug("Prepare has not completed yet, skipping heartbeat.")

Configuration

No configuration changes are anticipated.

Database

None

Public API

None

Public API Security

None

Python API

None

CLI (python-troveclient)

None

Internal API

The ServiceStatuses.BUILD_PENDING state has been renamed to ServiceStatuses.INSTANCE_READY to better reflect the instance’s actual state. The value displayed will remain ‘BUILD’ so that there should be no outward differences and thus backwards compatibility will be maintained.

Guest Agent

This change should not affect any behavior on the Guest Agent. The current tests should be adequate to ensure that the change is fully compatible with the rest of the code base.

Alternatives

One suggestion as to how to fix the prepare issue was to use Nova metadata, which is available on the guest instance. If it is decided that this would be useful, it could be added in addition to the proposed method as a means of notification only.

Dashboard Impact (UX)

TBD (section added after approval)

Implementation

Assignee(s)

Primary assignee:
<peterstac>

Milestones

Target Milestone for completion:
eg. Mitaka-1

Work Items

All changes will be done in the context of a single task.

Upgrade Implications

None are anticipated.

Dependencies

None

Testing

The unittests will be modified as necessary, however minimal changes will be made in this regard as well. In order to keep the changes as small as possible, refactoring the tests will also be done in stages with only the bare minimum done to start and the remainder being left to a future date. The future work would include testing the base class thoroughly and then removing all the corresponding tests from the derived classes.

The int-tests should continue to run as always, and will be used to determine that no fundamental changes have been made to the implementation, with the exception of the bug fixes (and they should just lead to greater stability in the test infrastructure).

Documentation Impact

Since all the changes are implementation related, no documentation changes are foreseen.

Appendix

None