Deprecate 'guest_log_long_query_time'

Slow query time should be configurable on per-instance basis
via the existing configuration group mechanism (see the bug).

We need to deprecate the existing global conf property first.
The property has been deprecated on all datastores.

Validation rules have been updated on those that
support log retrieval as well:

  - MySQL: long_query_time
  - Percona: long_query_time
  - Percona XtraDB Cluster: long_query_time
  - PostgreSQL: log_min_duration_statement
  - MariaDB: long_query_time

The logging settings will be saved with lower 'priority' than
any user settings. This will ensure the user settings
(if present) always override the global value.
Once the deprecated options go away we can again apply
the logging settings at the system level.

Change-Id: I9c28ceb933b6819b8e96556584b2e26cab2155fb
DocImpact: Document deprecated properties
Partial-Bug: 1542485

12 files changed, 87 insertions, 13 deletions

diff --git a/releasenotes/notes/deprecate-long-query-time-b85af24772e2e7cb.yaml b/releasenotes/notes/deprecate-long-query-time-b85af24772e2e7cb.yaml
new file mode 100644
index 00000000..2efdb77d
--- /dev/null
+++ b/releasenotes/notes/deprecate-long-query-time-b85af24772e2e7cb.yaml
@@ -0,0 +1,12 @@
+---
+deprecations:
+  - Make 'long query time' manageable via
+    configuration groups (see bug 1542485).
+    Deprecate the global 'guest_log_long_query_time'
+    option in preference of datastore-specific
+    configurations.
+    MySQL long_query_time
+    Percona long_query_time
+    Percona XtraDB Cluster long_query_time
+    MariaDB long_query_time
+    PostgreSQL log_min_duration_statement
diff --git a/trove/common/cfg.py b/trove/common/cfg.py
index 9b8876c5..d404a37e 100644
--- a/trove/common/cfg.py
+++ b/trove/common/cfg.py
@@ -548,7 +548,10 @@ mysql_opts = [
                help='List of Guest Logs to expose for publishing.'),
     cfg.IntOpt('guest_log_long_query_time', default=1000,
                help='The time in milliseconds that a statement must take in '
-                    'in order to be logged in the slow_query log.'),
+                    'in order to be logged in the slow_query log.',
+               deprecated_for_removal=True,
+               deprecated_reason='Will be replaced by a configuration group '
+               'option: long_query_time'),
     cfg.IntOpt('default_password_length', default=36,
                help='Character length of generated passwords.',
                deprecated_name='default_password_length',
@@ -632,7 +635,10 @@ percona_opts = [
                help='List of Guest Logs to expose for publishing.'),
     cfg.IntOpt('guest_log_long_query_time', default=1000,
                help='The time in milliseconds that a statement must take in '
-                    'in order to be logged in the slow_query log.'),
+                    'in order to be logged in the slow_query log.',
+               deprecated_for_removal=True,
+               deprecated_reason='Will be replaced by a configuration group '
+               'option: long_query_time'),
     cfg.IntOpt('default_password_length',
                default='${mysql.default_password_length}',
                help='Character length of generated passwords.',
@@ -721,7 +727,10 @@ pxc_opts = [
                help='List of Guest Logs to expose for publishing.'),
     cfg.IntOpt('guest_log_long_query_time', default=1000,
                help='The time in milliseconds that a statement must take in '
-                    'in order to be logged in the slow_query log.'),
+                    'in order to be logged in the slow_query log.',
+               deprecated_for_removal=True,
+               deprecated_reason='Will be replaced by a configuration group '
+               'option: long_query_time'),
     cfg.IntOpt('default_password_length',
                default='${mysql.default_password_length}',
                help='Character length of generated passwords.',
@@ -1102,7 +1111,10 @@ postgresql_opts = [
                help="The time in milliseconds that a statement must take in "
                     "in order to be logged in the 'general' log.  A value of "
                     "'0' logs all statements, while '-1' turns off "
-                    "statement logging."),
+                    "statement logging.",
+               deprecated_for_removal=True,
+               deprecated_reason='Will be replaced by configuration group '
+               'option: log_min_duration_statement'),
     cfg.IntOpt('default_password_length', default=36,
                help='Character length of generated passwords.',
                deprecated_name='default_password_length',
@@ -1378,7 +1390,10 @@ mariadb_opts = [
                help='List of Guest Logs to expose for publishing.'),
     cfg.IntOpt('guest_log_long_query_time', default=1000,
                help='The time in milliseconds that a statement must take in '
-                    'in order to be logged in the slow_query log.'),
+                    'in order to be logged in the slow_query log.',
+               deprecated_for_removal=True,
+               deprecated_reason='Will be replaced by a configuration group '
+               'option: long_query_time'),
     cfg.BoolOpt('cluster_support', default=True,
                 help='Enable clusters to be created and managed.'),
     cfg.IntOpt('min_cluster_member_count', default=3,
diff --git a/trove/configuration/service.py b/trove/configuration/service.py
index fc68146d..19dd51c3 100644
--- a/trove/configuration/service.py
+++ b/trove/configuration/service.py
@@ -311,6 +311,8 @@ class ConfigurationsController(wsgi.Controller):
             return six.string_types
         elif value_type == "integer":
             return six.integer_types
+        elif value_type == "float":
+            return float
         else:
             raise exception.TroveError(_(
                 "Invalid or unsupported type defined in the "
diff --git a/trove/guestagent/datastore/manager.py b/trove/guestagent/datastore/manager.py
index 36273212..0fd010f0 100644
--- a/trove/guestagent/datastore/manager.py
+++ b/trove/guestagent/datastore/manager.py
@@ -453,8 +453,8 @@ class Manager(periodic_task.PeriodicTasks):
         self.guest_log_context = context
         gl_cache = self.guest_log_cache
         result = filter(None, [gl_cache[log_name].show()
-                        if gl_cache[log_name].exposed else None
-                        for log_name in gl_cache.keys()])
+                               if gl_cache[log_name].exposed else None
+                               for log_name in gl_cache.keys()])
         LOG.info(_("Returning list of logs: %s") % result)
         return result
 
@@ -467,7 +467,7 @@ class Manager(periodic_task.PeriodicTasks):
         if publish and not disable:
             enable = True
         LOG.info(_("Processing guest log '%(log)s' "
-                 "(enable=%(en)s, disable=%(dis)s, "
+                   "(enable=%(en)s, disable=%(dis)s, "
                    "publish=%(pub)s, discard=%(disc)s).") %
                  {'log': log_name, 'en': enable, 'dis': disable,
                   'pub': publish, 'disc': discard})
@@ -554,8 +554,12 @@ class Manager(periodic_task.PeriodicTasks):
             config_man_values = cfg_values
             if section_label:
                 config_man_values = {section_label: cfg_values}
-            self.configuration_manager.apply_system_override(
-                config_man_values, change_id=apply_label)
+            # Applying the changes with a group id lower than the one used
+            # by user overrides. Any user defined value will override these
+            # settings (irrespective of order in which they are applied).
+            # See Bug 1542485
+            self.configuration_manager._apply_override(
+                '10-system-low-priority', apply_label, config_man_values)
         if restart_required:
             self.status.set_status(instance.ServiceStatuses.RESTART_REQUIRED)
         else:
diff --git a/trove/templates/mariadb/validation-rules.json b/trove/templates/mariadb/validation-rules.json
index c5c4111c..abebfe18 100644
--- a/trove/templates/mariadb/validation-rules.json
+++ b/trove/templates/mariadb/validation-rules.json
@@ -231,6 +231,12 @@
             "name": "performance_schema",
             "restart_required": true,
             "type": "boolean"
+        },
+        {
+            "name": "long_query_time",
+            "restart_required": false,
+            "min": 0,
+            "type": "float"
         }
     ]
 }
diff --git a/trove/templates/mysql/validation-rules.json b/trove/templates/mysql/validation-rules.json
index c5c4111c..abebfe18 100644
--- a/trove/templates/mysql/validation-rules.json
+++ b/trove/templates/mysql/validation-rules.json
@@ -231,6 +231,12 @@
             "name": "performance_schema",
             "restart_required": true,
             "type": "boolean"
+        },
+        {
+            "name": "long_query_time",
+            "restart_required": false,
+            "min": 0,
+            "type": "float"
         }
     ]
 }
diff --git a/trove/templates/percona/validation-rules.json b/trove/templates/percona/validation-rules.json
index acf00199..f1525588 100644
--- a/trove/templates/percona/validation-rules.json
+++ b/trove/templates/percona/validation-rules.json
@@ -219,6 +219,12 @@
             "name": "collation_server",
             "restart_required": false,
             "type": "string"
+        },
+        {
+            "name": "long_query_time",
+            "restart_required": false,
+            "min": 0,
+            "type": "float"
         }
     ]
 }
\ No newline at end of file
diff --git a/trove/templates/postgresql/validation-rules.json b/trove/templates/postgresql/validation-rules.json
index 702158ea..9a285eb7 100644
--- a/trove/templates/postgresql/validation-rules.json
+++ b/trove/templates/postgresql/validation-rules.json
@@ -898,6 +898,12 @@
             "name": "restart_after_crash",
             "restart_required": false,
             "type": "boolean"
+        },
+        {
+            "name": "log_min_duration_statement",
+            "restart_required": false,
+            "min": -1,
+            "type": "integer"
         }
     ]
 }
diff --git a/trove/templates/pxc/validation-rules.json b/trove/templates/pxc/validation-rules.json
index 8b7153fc..c1d8a970 100644
--- a/trove/templates/pxc/validation-rules.json
+++ b/trove/templates/pxc/validation-rules.json
@@ -219,6 +219,12 @@
             "name": "collation_server",
             "restart_required": false,
             "type": "string"
+        },
+        {
+            "name": "long_query_time",
+            "restart_required": false,
+            "min": 0,
+            "type": "float"
         }
     ]
 }
diff --git a/trove/tests/scenario/helpers/mysql_helper.py b/trove/tests/scenario/helpers/mysql_helper.py
index fcd0ba96..7fada681 100644
--- a/trove/tests/scenario/helpers/mysql_helper.py
+++ b/trove/tests/scenario/helpers/mysql_helper.py
@@ -46,7 +46,8 @@ class MysqlHelper(SqlHelper):
                 'join_buffer_size': 10485760}
 
     def get_non_dynamic_group(self):
-        return {'innodb_buffer_pool_size': 10485760}
+        return {'innodb_buffer_pool_size': 10485760,
+                'long_query_time': 59.1}
 
     def get_invalid_groups(self):
         return [{'key_buffer_size': -1}, {"join_buffer_size": 'string_value'}]
diff --git a/trove/tests/scenario/helpers/postgresql_helper.py b/trove/tests/scenario/helpers/postgresql_helper.py
index ffa9b8a5..5b0c37b2 100644
--- a/trove/tests/scenario/helpers/postgresql_helper.py
+++ b/trove/tests/scenario/helpers/postgresql_helper.py
@@ -45,7 +45,8 @@ class PostgresqlHelper(SqlHelper):
                  'databases': [{'name': 'db1'}, {'name': 'db2'}]}]
 
     def get_dynamic_group(self):
-        return {'effective_cache_size': '528MB'}
+        return {'effective_cache_size': '528MB',
+                'log_min_duration_statement': 257}
 
     def get_non_dynamic_group(self):
         return {'max_connections': 113}
diff --git a/trove/tests/scenario/runners/configuration_runners.py b/trove/tests/scenario/runners/configuration_runners.py
index 6d0a8101..916c6813 100644
--- a/trove/tests/scenario/runners/configuration_runners.py
+++ b/trove/tests/scenario/runners/configuration_runners.py
@@ -323,7 +323,16 @@ class ConfigurationRunner(TestRunner):
         host = self.get_instance_host(instance_id)
         for name, value in expected_configs.items():
             actual = self.test_helper.get_configuration_value(name, host)
-            self.assert_equal(str(value), str(actual),
+            # Compare floating point numbers as floats to avoid rounding
+            # and precision issues.
+            try:
+                expected_value = float(value)
+                actual_value = float(actual)
+            except ValueError:
+                expected_value = str(value)
+                actual_value = str(actual)
+
+            self.assert_equal(expected_value, actual_value,
                               "Unexpected value of property '%s'" % name)
 
     def run_list_dynamic_inst_conf_groups_after(self):