fixes saltstack/salt#63231 serious performance issues with the file.tidied module

changelog/63231.fixed

@@ -0,0 +1 @@
+Fix serious performance issues with the file.tidied module

salt/states/file.py

@@ -286,6 +286,7 @@ import os
 import posixpath
 import re
 import shutil
+import stat
 import sys
 import time
 import traceback
@@ -2073,15 +2074,18 @@ def tidied(
 
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
 
-    if age_size_logical_operator.upper() not in ["AND", "OR"]:
+    age_size_logical_operator = age_size_logical_operator.upper()
+    if age_size_logical_operator not in ["AND", "OR"]:
         age_size_logical_operator = "OR"
         log.warning("Logical operator must be 'AND' or 'OR'. Defaulting to 'OR'...")
 
-    if age_size_only and age_size_only.lower() not in ["age", "size"]:
+    if age_size_only:
-        age_size_only = None
+        age_size_only = age_size_only.lower()
-        log.warning(
+        if age_size_only not in ["age", "size"]:
-            "age_size_only parameter must be 'age' or 'size' if set. Defaulting to 'None'..."
+            age_size_only = None
-        )
+            log.warning(
+                "age_size_only parameter must be 'age' or 'size' if set. Defaulting to 'None'..."
+            )
 
     # Check preconditions
     if not os.path.isabs(name):
@@ -2134,44 +2138,41 @@ def tidied(
             path = os.path.join(root, elem)
             try:
                 if os.path.islink(path):
-                    # Get timestamp of symlink (not symlinked file)
-                    if time_comparison == "ctime":
-                        mytimestamp = os.lstat(path).st_ctime
-                    elif time_comparison == "mtime":
-                        mytimestamp = os.lstat(path).st_mtime
-                    else:
-                        mytimestamp = os.lstat(path).st_atime
                     if not rmlinks:
                         deleteme = False
+                    # Get info of symlink (not symlinked file)
+                    mystat = os.lstat(path)
                 else:
-                    # Get timestamp of file or directory
+                    mystat = os.stat(path)
-                    if time_comparison == "ctime":
-                        mytimestamp = os.path.getctime(path)
-                    elif time_comparison == "mtime":
-                        mytimestamp = os.path.getmtime(path)
-                    else:
-                        mytimestamp = os.path.getatime(path)
 
-                    if elem in dirs:
+                    if stat.S_ISDIR(mystat.st_mode):
                         # Check if directories should be deleted at all
                         deleteme = rmdirs
                     else:
                         # Get size of regular file
-                        mysize = os.path.getsize(path)
+                        mysize = mystat.st_size
+
+                if time_comparison == "ctime":
+                    mytimestamp = mystat.st_ctime
+                elif time_comparison == "mtime":
+                    mytimestamp = mystat.st_mtime
+                else:
+                    mytimestamp = mystat.st_atime
 
                 # Calculate the age and set the name to match
                 myage = abs(today - date.fromtimestamp(mytimestamp))
+
                 filename = elem
                 if full_path_match:
                     filename = path
 
                 # Verify against given criteria, collect all elements that should be removed
-                if age_size_only and age_size_only.lower() in ["age", "size"]:
+                if age_size_only and age_size_only in ["age", "size"]:
-                    if age_size_only.lower() == "age":
+                    if age_size_only == "age":
                         compare_age_size = myage.days >= age
                     else:
                         compare_age_size = mysize >= size
-                elif age_size_logical_operator.upper() == "AND":
+                elif age_size_logical_operator == "AND":
                     compare_age_size = mysize >= size and myage.days >= age
                 else:
                     compare_age_size = mysize >= size or myage.days >= age
@@ -2192,7 +2193,7 @@ def tidied(
         # Iterate over collected items
         try:
             for path in todelete:
-                __salt__["file.remove"](path, force=True)
+                __salt__["file.remove"](path)
                 ret["changes"]["removed"].append(path)
         except CommandExecutionError as exc:
             return _error(ret, "{}".format(exc))

tests/pytests/unit/states/file/test_tidied.py

@@ -1,7 +1,6 @@
 import logging
 import os
 from datetime import datetime, timedelta
-from types import SimpleNamespace
 
 import pytest
 
@@ -11,7 +10,7 @@ import salt.utils.json
 import salt.utils.platform
 import salt.utils.win_functions
 import salt.utils.yaml
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, PropertyMock, patch
 
 log = logging.getLogger(__name__)
 
@@ -33,11 +32,33 @@ def test__tidied():
     ]
     today_delta = datetime.today() - datetime.utcfromtimestamp(0)
     remove = MagicMock(name="file.remove")
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -57,15 +78,15 @@ def test__tidied():
         "result": True,
         "comment": "Removed 3 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 3
 
     remove.reset_mock()
+    mock_st_mode.reset_mock()
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -88,7 +109,7 @@ def test__tidied():
         "result": True,
         "comment": "Removed 6 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 6
 
 
@@ -120,12 +141,41 @@ def test_tidied_with_exclude():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = datetime.today() - datetime.utcfromtimestamp(0)
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -144,15 +194,13 @@ def test_tidied_with_exclude():
         "result": True,
         "comment": "Removed 2 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 2
 
     remove.reset_mock()
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -174,15 +222,13 @@ def test_tidied_with_exclude():
         "result": True,
         "comment": "Removed 5 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 5
 
     remove.reset_mock()
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -212,7 +258,7 @@ def test_tidied_with_exclude():
         "result": True,
         "comment": "Removed 6 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 6
 
 
@@ -227,12 +273,41 @@ def test_tidied_with_full_path_exclude():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = datetime.today() - datetime.utcfromtimestamp(0)
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -255,15 +330,13 @@ def test_tidied_with_full_path_exclude():
         "result": True,
         "comment": "Removed 2 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 2
 
     remove.reset_mock()
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -290,15 +363,13 @@ def test_tidied_with_full_path_exclude():
         "result": True,
         "comment": "Removed 5 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 5
 
     remove.reset_mock()
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -326,7 +397,7 @@ def test_tidied_with_full_path_exclude():
         "result": True,
         "comment": "Removed 6 files or directories from directory {}".format(name),
     }
-    assert exp == ret
+    assert ret == exp
     assert remove.call_count == 6
 
 
@@ -382,12 +453,29 @@ def test_tidied_age_size_args_AND_operator_age_not_size_age_only():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = (datetime.today() - timedelta(days=14)) - datetime.utcfromtimestamp(0)
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -470,12 +558,29 @@ def test_tidied_age_size_args_AND_operator_size_not_age_size_only():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = (datetime.today() - timedelta(days=14)) - datetime.utcfromtimestamp(0)
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -517,12 +622,29 @@ def test_tidied_age_size_args_AND_operator_size_and_age():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = (datetime.today() - timedelta(days=14)) - datetime.utcfromtimestamp(0)
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", return_value=False
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.stat", return_value=mystat), patch.dict(
-        "os.path.getsize", return_value=10
-    ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(
         filestate.__salt__, {"file.remove": remove}
@@ -592,14 +714,34 @@ def test_tidied_rmlinks():
         ("test", ["test1", "test2"], ["file3"]),
     ]
     today_delta = (datetime.today() - timedelta(days=14)) - datetime.utcfromtimestamp(0)
-    mock_lstat = MagicMock(return_value=SimpleNamespace(st_atime=today_delta))
+
+    mystat = MagicMock()
+    mystat.st_atime = today_delta.total_seconds()
+    # dir = 16877
+    # file = 33188
+    mock_st_mode = PropertyMock(
+        side_effect=[
+            33188,
+            16877,
+            33188,
+            16877,
+            16877,
+        ]
+    )
+    type(mystat).st_mode = mock_st_mode
+    mystat.st_size = 10
+
+    mylstat = MagicMock()
+    mylstat.st_atime = today_delta.total_seconds()
+    mylstat.st_mode = 33188
+    mylstat.st_size = 10
+
     remove = MagicMock(name="file.remove")
+
     with patch("os.walk", return_value=walker), patch(
         "os.path.islink", side_effect=[False, True, False, False, False, False]
-    ), patch("os.path.getatime", return_value=today_delta.total_seconds()), patch(
+    ), patch("os.lstat", return_value=mylstat), patch(
-        "os.lstat", return_value=mock_lstat
+        "os.stat", return_value=mystat
-    ), patch(
-        "os.path.getsize", return_value=10
     ), patch.dict(
         filestate.__opts__, {"test": False}
     ), patch.dict(