Rename idle_timeout to connection_recycle_time

The "idle_timeout" name and documentation are misleading to users.
Rename the option to connection_recycle_time to more closely
match that of the underlying SQLAlchemy argument name
"pool_recycle", which refers to a time period for which
a connection present in the pool for longer than that number
of seconds will be closed and replaced with a new one *upon checkout*,
to prevent usage of stale connections that may have been closed by
server-side reapers.

Change-Id: I5e39d5ac81eaa8d85b1dfb18862fe27695ab8b96

7 files changed, 59 insertions, 20 deletions

diff --git a/oslo_db/options.py b/oslo_db/options.py
index 824661f..8bd70a2 100644
--- a/oslo_db/options.py
+++ b/oslo_db/options.py
@@ -48,15 +48,25 @@ database_opts = [
                 default=False,
                 help='If True, transparently enables support for handling '
                      'MySQL Cluster (NDB).'),
-    cfg.IntOpt('idle_timeout',
-               default=3600,
-               deprecated_opts=[cfg.DeprecatedOpt('sql_idle_timeout',
-                                                  group='DEFAULT'),
-                                cfg.DeprecatedOpt('sql_idle_timeout',
-                                                  group='DATABASE'),
-                                cfg.DeprecatedOpt('idle_timeout',
-                                                  group='sql')],
-               help='Timeout before idle SQL connections are reaped.'),
+    cfg.IntOpt(
+        'connection_recycle_time',
+        default=3600,
+        deprecated_opts=[
+            cfg.DeprecatedOpt('idle_timeout',
+                              group="DATABASE"),
+            cfg.DeprecatedOpt('idle_timeout',
+                              group="database"),
+            cfg.DeprecatedOpt('sql_idle_timeout',
+                              group='DEFAULT'),
+            cfg.DeprecatedOpt('sql_idle_timeout',
+                              group='DATABASE'),
+            cfg.DeprecatedOpt('idle_timeout',
+                              group='sql')
+        ],
+        help='Connections which have been present in the connection '
+             'pool longer than this number of seconds will be replaced '
+             'with a new one the next time they are checked out from '
+             'the pool.'),
     cfg.IntOpt('min_pool_size',
                default=1,
                deprecated_opts=[cfg.DeprecatedOpt('sql_min_pool_size',
diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py
index 752c6ca..8213080 100644
--- a/oslo_db/sqlalchemy/enginefacade.py
+++ b/oslo_db/sqlalchemy/enginefacade.py
@@ -18,7 +18,8 @@ import operator
 import threading
 import warnings
 
-import debtcollector.removals as removals
+import debtcollector.removals
+import debtcollector.renames
 from oslo_config import cfg
 from oslo_utils import excutils
 
@@ -136,7 +137,7 @@ class _TransactionFactory(object):
             'sqlite_fk': _Default(False),
             'mysql_sql_mode': _Default('TRADITIONAL'),
             'mysql_enable_ndb': _Default(False),
-            'idle_timeout': _Default(3600),
+            'connection_recycle_time': _Default(3600),
             'connection_debug': _Default(0),
             'max_pool_size': _Default(),
             'max_overflow': _Default(),
@@ -177,6 +178,8 @@ class _TransactionFactory(object):
         self._legacy_facade = None
         self._start_lock = threading.Lock()
 
+    @debtcollector.renames.renamed_kwarg(
+        "idle_timeout", "connection_recycle_time", replace=True)
     def configure_defaults(self, **kw):
         """Apply default configurational options.
 
@@ -209,7 +212,7 @@ class _TransactionFactory(object):
 
         :param mysql_enable_ndb: enable MySQL Cluster (NDB) support
 
-        :param idle_timeout: connection pool recycle time,
+        :param connection_recycle_time: connection pool recycle time,
          defaults to 3600. Note the connection does not actually have to
          be "idle" to be recycled.
 
@@ -282,6 +285,8 @@ class _TransactionFactory(object):
         """
         self._configure(True, kw)
 
+    @debtcollector.renames.renamed_kwarg(
+        "idle_timeout", "connection_recycle_time", replace=True)
     def configure(self, **kw):
         """Apply configurational options.
 
@@ -529,7 +534,7 @@ class _TestTransactionFactory(_TransactionFactory):
 
     """
 
-    @removals.removed_kwarg(
+    @debtcollector.removals.removed_kwarg(
         'synchronous_reader',
         'argument value is propagated from the parent _TransactionFactory')
     def __init__(self, engine, maker, apply_global, from_factory=None, **kw):
@@ -1196,8 +1201,8 @@ class LegacyEngineFacade(object):
     :keyword mysql_enable_ndb: If True, transparently enables support for
                                handling MySQL Cluster (NDB).
                                (defaults to False)
-    :keyword idle_timeout: timeout before idle sql connections are reaped
-                           (defaults to 3600)
+    :keyword connection_recycle_time: Time period for connections to be
+                            recycled upon checkout (defaults to 3600)
     :keyword connection_debug: verbosity of SQL debugging information.
                                -1=Off, 0=None, 100=Everything (defaults
                                to 0)
@@ -1230,7 +1235,6 @@ class LegacyEngineFacade(object):
             "oslo_db.sqlalchemy.enginefacade",
             exception.OsloDBDeprecationWarning,
             stacklevel=2)
-
         if _factory:
             self._factory = _factory
         else:
diff --git a/oslo_db/sqlalchemy/engines.py b/oslo_db/sqlalchemy/engines.py
index 8cbd36c..2808ef4 100644
--- a/oslo_db/sqlalchemy/engines.py
+++ b/oslo_db/sqlalchemy/engines.py
@@ -22,6 +22,7 @@ import os
 import re
 import time
 
+import debtcollector.renames
 import six
 import sqlalchemy
 from sqlalchemy import event
@@ -121,9 +122,11 @@ def _vet_url(url):
             )
 
 
+@debtcollector.renames.renamed_kwarg(
+    "idle_timeout", "connection_recycle_time", replace=True)
 def create_engine(sql_connection, sqlite_fk=False, mysql_sql_mode=None,
                   mysql_enable_ndb=False,
-                  idle_timeout=3600,
+                  connection_recycle_time=3600,
                   connection_debug=0, max_pool_size=None, max_overflow=None,
                   pool_timeout=None, sqlite_synchronous=True,
                   connection_trace=False, max_retries=10, retry_interval=10,
@@ -137,7 +140,7 @@ def create_engine(sql_connection, sqlite_fk=False, mysql_sql_mode=None,
     _vet_url(url)
 
     engine_args = {
-        "pool_recycle": idle_timeout,
+        "pool_recycle": connection_recycle_time,
         'convert_unicode': True,
         'connect_args': {},
         'logging_name': logging_name
diff --git a/oslo_db/tests/sqlalchemy/test_enginefacade.py b/oslo_db/tests/sqlalchemy/test_enginefacade.py
index 303a8dd..7a7484f 100644
--- a/oslo_db/tests/sqlalchemy/test_enginefacade.py
+++ b/oslo_db/tests/sqlalchemy/test_enginefacade.py
@@ -2156,7 +2156,8 @@ class ConfigOptionsTest(oslo_test_base.BaseTestCase):
         with warnings.catch_warnings(record=True) as w:
             warnings.simplefilter("always")
 
-            factory.configure(fake1='x', idle_timeout=200, wrong2='y')
+            factory.configure(
+                fake1='x', connection_recycle_time=200, wrong2='y')
 
         self.assertEqual(1, len(w))
         self.assertTrue(
diff --git a/oslo_db/tests/sqlalchemy/test_options.py b/oslo_db/tests/sqlalchemy/test_options.py
index 2f3479d..45a29b1 100644
--- a/oslo_db/tests/sqlalchemy/test_options.py
+++ b/oslo_db/tests/sqlalchemy/test_options.py
@@ -84,6 +84,7 @@ pool_timeout=7
                                        ]])[0]
         self.conf(['--config-file', path])
         self.assertEqual('fake_connection', self.conf.database.connection)
+        self.assertEqual(100, self.conf.database.connection_recycle_time)
         self.assertEqual(100, self.conf.database.idle_timeout)
         self.assertEqual(99, self.conf.database.min_pool_size)
         self.assertEqual(199, self.conf.database.max_pool_size)
@@ -99,6 +100,7 @@ pool_timeout=7
                                        ]])[0]
         self.conf(['--config-file', path])
         self.assertEqual('test_sql_connection', self.conf.database.connection)
+        self.assertEqual(99, self.conf.database.connection_recycle_time)
         self.assertEqual(99, self.conf.database.idle_timeout)
 
     def test_deprecated_dbapi_parameters(self):
diff --git a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
index c9f3d7d..4ae2506 100644
--- a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
+++ b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
@@ -316,6 +316,10 @@ class EngineFacadeTestCase(oslo_test.BaseTestCase):
         self.assertFalse(ses.autocommit)
         self.assertTrue(ses.expire_on_commit)
 
+    def test_direct_invocation_deprecated_args(self):
+        facade = session.EngineFacade("sqlite://", idle_timeout=59)
+        self.assertEqual(59, facade.get_engine().pool._recycle)
+
     @mock.patch('oslo_db.sqlalchemy.orm.get_maker')
     @mock.patch('oslo_db.sqlalchemy.engines.create_engine')
     def test_creation_from_config(self, create_engine, get_maker):
@@ -343,7 +347,7 @@ class EngineFacadeTestCase(oslo_test.BaseTestCase):
             mysql_sql_mode='TRADITIONAL',
             mysql_enable_ndb=False,
             sqlite_fk=False,
-            idle_timeout=mock.ANY,
+            connection_recycle_time=mock.ANY,
             retry_interval=mock.ANY,
             max_retries=mock.ANY,
             max_overflow=mock.ANY,
diff --git a/releasenotes/notes/deprecate_idle_timeout-029d9f2cb7184b28.yaml b/releasenotes/notes/deprecate_idle_timeout-029d9f2cb7184b28.yaml
new file mode 100644
index 0000000..4e02e7a
--- /dev/null
+++ b/releasenotes/notes/deprecate_idle_timeout-029d9f2cb7184b28.yaml
@@ -0,0 +1,15 @@
+---
+deprecations:
+  - |
+    The configuration option ``idle_timeout`` is now deprecated and has been
+    renamed to  ``connection_recycle_time``, including within the main oslo.db
+    options, as well as in the keyword arguments to
+    ``engines.create_engine()``, ``enginefacade.configure()`` and
+    ``enginefacade.configure_defaults()``. The new name more accurately
+    describes what this option does, in that it is not directly related to the
+    "idle" time of the connection itself, nor is the connection disconnected at
+    any specific time.  It refers to a rule stating that any connection which
+    has been present more than N seconds as a member of the connection pool
+    will be automatically discarded and replaced the next time it is checked
+    out from the pool.
+