[BUG] MySQL module not closing connections/cursors

[lvinagre]

Description
While using MySQL module to manage users/grants in CloudSQL (GCP), it has been noticed connections/cursors are not being closed by end of execution and are kept open until timeout occurs.

Setup
salt-master running on GCP VM instance (Debian 11)
CloudSQL MySQL instance (8.0.31-google)

Please be as specific as possible and give set-up details.

• on-prem machine
• VM (Virtualbox, KVM, etc. please specify)
• VM running on a cloud service, please be explicit and add details
• container (Kubernetes, Docker, containerd, etc. please specify)
• or a combination, please be explicit: MySQL instance on CloudSQL (GCP)
• jails if it is FreeBSD
• classic packaging
• onedir packaging
• used bootstrap to install

Steps to Reproduce the behavior

• Create a state to add a user and grants to that user
• Let it run and check GCP logs afterwards (from our setup, they appear after 30~45s after):

severity: "INFO"
textPayload: "2025-05-17T01:51:33.854362Z 2365374 [Note] [MY-010914] [Server] Aborted connection 2365374 to db: 'unconnected' user: '{USER}' host: '{SOURCE IP}' (Got an error reading communication packets)."

Expected behavior
No errors/timeouts are logged after state finishes execution

Versions Report

Salt Version:
          Salt: 3006.7

Python Version:
        Python: 3.10.13 (main, Feb 19 2024, 03:31:20) [GCC 11.2.0]

Dependency Versions:
          cffi: 1.14.6
      cherrypy: 18.6.1
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: 4.0.11
     gitpython: 3.1.41
        Jinja2: 3.1.3
       libgit2: Not Installed
  looseversion: 1.0.2
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 22.0
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.19.1
        pygit2: Not Installed
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 23.2.0
        relenv: 0.15.1
         smmap: 5.0.1
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist: debian 11 bullseye
        locale: utf-8
       machine: x86_64
       release: 5.10.0-25-cloud-amd64
        system: Linux
       version: Debian GNU/Linux 11 bullseye

Additional context
Looking at the module's code, I do not see any connection or cursor being handled using python's with nor being closed after being opened/used. Not sure if this was deemed not necessary, but I would say it is nice to have it.

[lkubb]

I'm not overly familiar with the design of this extension, so this is a general remark from someone who has written a lot of Salt extension modules, including several ones involving clients (Vault, Podman, InfluxDB v2, Snapd, ...):

It's quite hard to do connection management inside a Salt extension module if you don't want to create a fresh, separate connection for each single execution module call. Using context managers is not an issue if your module isn't usually called many times during a single execution, but would make integrations like the Vault one noticeably slower.

[lvinagre]

@lkubb, let me take a look at the code a bit more and do some test, but I feel that this module is doing exactly what you've just mentioned: "create a fresh, separate connection for each single execution module call". Maybe I missed something. I'll post back here.

[lvinagre]

Hey @lkubb, I tried reading the mysql module again just to make sure I was not missing something you were trying to mention, but it seems to me the connections are not being pooled (pretty sure on that one) nor a connection being used for multiple operations. To try to check my thoughts are in the right place I added a log.debug(f"MySQL open connection: {id(dbc)}") to the try block that handles the dbc = MySQLdb.connect(**connargs) and compared the numbers there (762) to the total of states in our state file (reported when we run it: 662). Considering that some of the mysql module's functions do more than one call to the DB instance itself, I would say we are at a 1:1 for connection to function execution right now.

$ salt-call state.apply queue=true {salt,pillar}env=lvinagre/mysql mysql.user
...
Total states run:     662
Total run time:     3.612 s

$ salt-call -l debug state.apply queue=true {salt,pillar}env=lvinagre/mysql mysql.users 2>&1 | grep 'open connection' | sort | uniq -c | wc -l
762

With all that in mind, the reason which I opened this issue was to have those 762+ connections closed after they are used/finish their intended operation.

[lkubb]

Thanks for investigating this @lvinagre ! My comment was meant as guidance rather than rejection of the premise, sorry if it caused confusion.

Indeed, looking through the code of the execution module, it seems it combines the worst of both options - it does not close connections while still creating a new one for each call. Do you intend to work on a fix for this?

The pillar module actually makes use of a context manager:

saltext-mysql/src/saltext/mysql/pillar/mysql.py

Lines 112 to 132 in bf8eb59

	@contextmanager
	def _get_cursor(self):
	"""
	Yield a MySQL cursor
	"""
	_options = self._get_options()
	conn = MySQLdb.connect(
	host=_options["host"],
	user=_options["user"],
	passwd=_options["pass"],
	db=_options["db"],
	port=_options["port"],
	ssl=_options["ssl"],
	)
	cursor = conn.cursor()
	try:
	yield cursor
	except MySQLdb.DatabaseError as err:
	log.exception("Error in ext_pillar MySQL: %s", err.args)
	finally:
	conn.close()

The returner module caches the connection in the context and makes use of a context manager for cursors only:

saltext-mysql/src/saltext/mysql/returners/mysql.py

Lines 229 to 244 in bf8eb59

	@contextmanager
	def _get_serv(ret=None, commit=False):
	"""
	Return a mysql cursor
	"""
	_options = _get_options(ret)
	connect = True
	if __context__ and "mysql_returner_conn" in __context__:
	try:
	log.debug("Trying to reuse MySQL connection pool")
	conn = __context__["mysql_returner_conn"]
	conn.ping()
	connect = False
	except OperationalError as exc:
	log.debug("OperationalError on ping: %s", exc)

The cache module similarly makes use of the context:

saltext-mysql/src/saltext/mysql/cache/mysql_cache.py

Lines 223 to 226 in bf8eb59

	def _init_client():
	"""Initialize connection and create table if needed"""
	if __context__.get("mysql_client") is not None:
	return

The auth module on the other hand works like the execution module:

saltext-mysql/src/saltext/mysql/auth/mysql.py

Lines 101 to 119 in bf8eb59

	def auth(username, password):
	"""
	Authenticate using a MySQL user table
	"""
	_info = __get_connection_info()
	if _info is None:
	return False
	try:
	conn = MySQLdb.connect(
	_info["hostname"], _info["username"], _info["password"], _info["database"]
	)
	except OperationalError as e:
	log.error(e)
	return False
	cur = conn.cursor()
	cur.execute(_info["auth_sql"].format(username, password))

If I had to guess, allowing connection parameter overrides per execution module call was seen to complicate the context cache a bit. I'd still prefer using the context as a cache per connection parameter tuple to avoid creating hundreds of separate connections, similar to this mockup:

CONN_CKEY = "_mysql_conn"


def _connect(**kwargs):
    # [...]

    ckey = tuple(sorted(connargs.items()))
    # unsure if we need this here, likely depends on the highstate runtime
    if ckey in __context__.setdefault(CONN_CKEY, {}):
        try:
            log.debug("Trying to reuse MySQL connection pool")
            conn = __context__[CONN_CKEY][ckey]
            conn.ping()
        except OperationalError as exc:
            __context__[CONN_CKEY].pop(ckey)
            log.debug("OperationalError on ping: %s", exc)

    if ckey not in __context__.setdefault(CONN_CKEY, {}):
        try:
            dbc = MySQLdb.connect(**connargs)
        except (MySQLdb.err.InternalError, OperationalError) as exc:
            err = "MySQL Error {}: {}".format(*exc.args)
            __context__["mysql.error"] = err
            log.error(err)
            return None
        __context__[CONN_CKEY][ckey] = dbc
    return __context__[CONN_CKEY][ckey]

If this doesn't work as simple as this for some reason (if some connargs can cause the tuple to be unhashable, for example) or you think closing and reopening each connection would be preferable, we can still go with the context manager variant.

Minor unrelated remark: Centralizing the connection creation and caching in a shared utils module would be more DRY.

[lvinagre]

@lkubb, I can try to, but will probably require some time for me to understand the examples you posted and the time to actually work on the fix (if I have the knowledge to).