From e0d241fa4d593934d228f4b94c750752c2e60d3b Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 28 Apr 2021 17:23:56 -0700 Subject: [PATCH] Ironic index docs/command check backport Secure RBAC, along with numerous periodics, and even some common API access patterns heavily access the ironic database and sometimes cause queries across multiple columns to match nodes to return. This change is largely intended to provide the upgrade check call as well as the documentation in the Wallaby release of ironic so operators can locate it and add the indexes prior to the upgrade to ironic 18.0. Note: This change includes the content from Ifa9da65dc8df5bf9a369d6faeab310386dfd944a which includes a fix for the status command and the upgrade script as to properly identify and raise visibility, where the original change to the status command failed to include the actual value in the field to cause the standard execution by the upgrade script. Also includes the same upgrade check handling for ironic-status command in the stable branch devstack plugin as this change does raise an informational warning which is non-blocking to ironic's use on this branch to signal that action may be advisable. Story: 2008863 Task: 42392 Change-Id: I76821c032180a58d0f99d31110fbe0f844c0cb3f (cherry picked from commit 205a8b3037b62cbae7f5385f4d9b9404a702414e) --- devstack/lib/ironic | 9 ++++- devstack/upgrade/upgrade.sh | 10 ++++- doc/source/admin/tuning.rst | 76 +++++++++++++++++++++++++++++++++++++ ironic/cmd/status.py | 37 ++++++++++++++++++ 4 files changed, 130 insertions(+), 2 deletions(-) diff --git a/devstack/lib/ironic b/devstack/lib/ironic index e6aa8d8651..1936c5ad73 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -1841,7 +1841,14 @@ function init_ironic { # NOTE(rloo): We're not upgrading but want to make sure this command works, # even though we're not parsing the output of this command. - $IRONIC_BIN_DIR/ironic-status upgrade check + $IRONIC_BIN_DIR/ironic-status upgrade check && ret_val=$? || ret_val=$? + if [ $ret_val -gt 1 ]; then + # NOTE(TheJulia): The ironic-status check command can return + # 0 if all is okay, 1 if there is a warning, or >1 if something + # is broken on a major upgrade and thus the process needs to + # be aborted. + die $LINENO "Ironic db status check failed, returned: $ret_val" + fi } # _ironic_bm_vm_names() - Generates list of names for baremetal VMs. diff --git a/devstack/upgrade/upgrade.sh b/devstack/upgrade/upgrade.sh index 5e70ee4314..8c6096da83 100755 --- a/devstack/upgrade/upgrade.sh +++ b/devstack/upgrade/upgrade.sh @@ -74,7 +74,15 @@ upgrade_project ironic $RUN_DIR $BASE_DEVSTACK_BRANCH $TARGET_DEVSTACK_BRANCH # NOTE(rloo): make sure it is OK to do an upgrade. Except that we aren't # parsing/checking the output of this command because the output could change # based on the checks it makes. -$IRONIC_BIN_DIR/ironic-status upgrade check +$IRONIC_BIN_DIR/ironic-status upgrade check && ret_val=$? || ret_val=$? +if [ $ret_val -gt 1 ] ; then + # NOTE(TheJulia): We need to evaluate the return code from the + # upgrade status check as the framework defines + # Warnings are permissible and returned as status code 1, errors are + # returned as greater than 1 which means there is a major upgrade + # stopping issue which needs to be addressed. + die $LINENO "Ironic DB Status check failed, returned: $ret_val" +fi $IRONIC_BIN_DIR/ironic-dbsync --config-file=$IRONIC_CONF_FILE diff --git a/doc/source/admin/tuning.rst b/doc/source/admin/tuning.rst index 02636e6083..c7beb5e6e3 100644 --- a/doc/source/admin/tuning.rst +++ b/doc/source/admin/tuning.rst @@ -113,6 +113,82 @@ perform ten concurrent deployments of images requiring conversion, the memory needed may exceed 10GB. This does however, entirely depend upon image block structure and layout, and what deploy interface is being used. +Database +======== + +Query load upon the database is one of the biggest potential bottlenecks which +can cascade across a deployment and ultimately degrade service to an Ironic +user. + +Often, depending on load, query patterns, periodic tasks, and so on and so +forth, additional indexes may be needed to help provide hints to the database +so it can most efficently attempt to reduce the number of rows which need to +be examined in order to return a result set. + +Adding indexes +-------------- + +This example below is specific to MariaDB/MySQL, but the syntax should be +easy to modify for operators using PostgreSQL. + +.. code-block:: sql + + use ironic; + create index owner_idx on nodes (owner) LOCK = SHARED; + create index lessee_idx on nodes (lessee) LOCK = SHARED; + create index driver_idx on nodes (driver) LOCK = SHARED; + create index provision_state_idx on nodes (provision_state) LOCK = SHARED; + create index reservation_idx on nodes (reservation) LOCK = SHARED; + create index conductor_group_idx on nodes (conductor_group) LOCK = SHARED; + create index resource_class_idx on nodes (resource_class) LOCK = SHARED; + +.. note:: The indexes noted have been added automatically by Xena versions of + Ironic and later. They are provided here as an example and operators can + add them manually prior with versions of Ironic. The database upgrade for + the Xena release of Ironic which adds these indexes are only aware of being + able to skip index creation if it already exists on MySQL/MariaDB. + +.. note:: It may be possible to use "LOCK = NONE". Basic testing indicates + this takes a little bit longer, but shouldn't result in the database + table becoming write locked during the index creation. If the database + engine cannot support this, then the index creation will fail. + +Database platforms also have a concept of what is called a "compound index" +where the index is aligned with the exact query pattern being submitted to +the database. The database is able to use this compound index to attempt to +drastically reduce the result set generation time for the remainder of the +query. As of the composition of this document, we do not ship compound +indexes in Ironic as we feel the most general benefit is single column +indexes, and depending on data present, an operator may wish to explore +compound indexes with their database administrator, as comound indexes +can also have negative performance impacts if improperly constructed. + +.. code-block:: sql + + use ironic; + create index my_custom_app_query_index on nodes (reservation, provision_state, driver); + +The risk, and *WHY* you should engage a Database Administrator, is depending on +your configuration, the actual index may need to include one or more additional +fields such as owner or lessee which may be added on to the index. At the same +time, queries with less field matches, or in different orders will exhibit +different performance as the compound index may not be able to be consulted. + +Indexes will not fix everything +------------------------------- + +Indexes are not a magical cure-all for all API or database performance issues, +but they are an increadibly important part depending on data access and query +patterns. + +The underlying object layer and data conversions including record pagination +do add a substantial amount of overhead to what may otherwise return as a +result set on a manual database query. In Ironic's case, due to the object +model and the need to extract multiple pieces of data at varying levels +of the data model to handle cases such as upgrades, the entire result set +is downloaded and transformed which is an overhead you do not experience with +a command line database client. + What can I do? ============== diff --git a/ironic/cmd/status.py b/ironic/cmd/status.py index f4ab695001..37fdbaf7dd 100644 --- a/ironic/cmd/status.py +++ b/ironic/cmd/status.py @@ -15,6 +15,8 @@ import sys from oslo_config import cfg +from oslo_db.sqlalchemy import enginefacade +from oslo_db.sqlalchemy import utils from oslo_upgradecheck import common_checks from oslo_upgradecheck import upgradecheck @@ -49,6 +51,40 @@ class Checks(upgradecheck.UpgradeCommands): else: return upgradecheck.Result(upgradecheck.Code.FAILURE, details=msg) + def _check_db_indexes(self): + """Check if indexes exist on heavily used columns. + + Checks the database to see if indexes exist on heavily used columns + and provide guidance of action that can be taken to improve ironic + database performance. + """ + engine = enginefacade.reader.get_engine() + + indexes = [ + ('nodes', 'reservation_idx'), + ('nodes', 'driver_idx'), + ('nodes', 'provision_state_idx'), + ('nodes', 'conductor_group_idx'), + ('nodes', 'resource_class_idx'), + ('nodes', 'reservation_idx'), + ('nodes', 'owner_idx'), + ('nodes', 'lessee_idx'), + ] + missing_indexes = [] + for table, idx in indexes: + if not utils.index_exists(engine, table, idx): + missing_indexes.append(idx) + + if missing_indexes: + idx_list = ', '.join(missing_indexes) + msg = ('Indexes missing for ideal database performance. Please ' + 'consult https://docs.openstack.org/ironic/latest/admin/' + 'tuning.html for information on indexes. Missing: %s' + % idx_list) + return upgradecheck.Result(upgradecheck.Code.WARNING, details=msg) + else: + return upgradecheck.Result(upgradecheck.Code.SUCCESS) + # A tuple of check tuples of (, ). # The name of the check will be used in the output of this command. # The check function takes no arguments and returns an @@ -59,6 +95,7 @@ class Checks(upgradecheck.UpgradeCommands): # summary will be rolled up at the end of the check() method. _upgrade_checks = ( (_('Object versions'), _check_obj_versions), + (_('Database Index Status'), _check_db_indexes), # Victoria -> Wallaby migration (_('Policy File JSON to YAML Migration'), (common_checks.check_policy_json, {'conf': CONF})),