launchpad-reviewers team mailing list archive

[İlker Emre Koç <mp+492122@xxxxxxxxxxxxxxxxxx>]

İlker Emre Koç has proposed merging ~ilkeremrekoc/launchpad:buildbot-memory-fix-modification into launchpad:master.

Commit message:
Add a memory preserve context manager to a missed test

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilkeremrekoc/launchpad/+git/launchpad/+merge/492122

Another test, this time inside the BranchScanJob, was silently
modifying the memory limit, as such, we wrapped it with
"preserve_limit()".

To avoid these memory changes, the preserve_limit context manager first
saves the current RLIMIT_AS resource, which denotes the virtual memory
limit, and then resets it to the saved value after the run has
completed.

Also added more descriptive explanations and reduced the scope of
the error catch within the new context manager.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilkeremrekoc/launchpad:buildbot-memory-fix-modification into launchpad:master.

diff --git a/lib/lp/code/model/tests/test_branchjob.py b/lib/lp/code/model/tests/test_branchjob.py
index ea830a3..62e1010 100644
--- a/lib/lp/code/model/tests/test_branchjob.py
+++ b/lib/lp/code/model/tests/test_branchjob.py
@@ -6,6 +6,7 @@
 import os
 import shutil
 from datetime import datetime, timedelta, timezone
+from resource import RLIMIT_AS  # Maximum memory that can be taken by a process
 from typing import Optional
 
 import transaction
@@ -63,7 +64,7 @@ from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import JobRunner
 from lp.services.job.tests import block_on_job
-from lp.services.osutils import override_environ
+from lp.services.osutils import override_environ, preserve_rlimit
 from lp.services.webapp import canonical_url
 from lp.testing import TestCaseWithFactory, person_logged_in
 from lp.testing.dbuser import dbuser, switch_dbuser
@@ -210,7 +211,11 @@ class TestBranchScanJob(TestCaseWithFactory):
             MockPatch("lp.code.model.branchjob.BranchScanJob.run", mock_run)
         )
         runner = JobRunner([job])
-        with dbuser("branchscanner"):
+
+        # Preserve the virtual memory resource limit because runJobHandleError
+        # changes it which causes the whole test worker process to have
+        # limited memory
+        with dbuser("branchscanner"), preserve_rlimit(RLIMIT_AS):
             runner.runJobHandleError(job)
         self.assertEqual(1, len(self.oopses))
         actions = [action[2:4] for action in self.oopses[0]["timeline"]]
diff --git a/lib/lp/crafts/tests/test_craftrecipebuildjob.py b/lib/lp/crafts/tests/test_craftrecipebuildjob.py
index ebddc16..6b95efd 100644
--- a/lib/lp/crafts/tests/test_craftrecipebuildjob.py
+++ b/lib/lp/crafts/tests/test_craftrecipebuildjob.py
@@ -8,7 +8,7 @@ import os
 import tarfile
 import tempfile
 from pathlib import Path
-from resource import RLIMIT_AS
+from resource import RLIMIT_AS  # Maximum memory that can be taken by a process
 
 from artifactory import ArtifactoryPath
 from fixtures import FakeLogger
@@ -98,8 +98,8 @@ class TestCraftPublishingJob(TestCaseWithFactory):
         """Helper to run a job and return the result."""
         job = getUtility(ICraftPublishingJobSource).create(self.build)
 
-        # Preserve RLIMIT_AS resource limit because runAll changes it which
-        # causes the whole test worker process to have limited memory
+        # Preserve the virtual memory resource limit because runAll changes it
+        # which causes the whole test worker process to have limited memory
         with preserve_rlimit(RLIMIT_AS):
             JobRunner([job]).runAll()
 
@@ -859,8 +859,8 @@ class TestCraftPublishingJob(TestCaseWithFactory):
             lambda self: "https://example.com/repo.git";,
         )
 
-        # Preserve RLIMIT_AS resource limit because runAll changes it which
-        # causes the whole test worker process to have limited memory
+        # Preserve the virtual memory resource limit because runAll changes it
+        # which causes the whole test worker process to have limited memory
         with preserve_rlimit(RLIMIT_AS):
             JobRunner([job]).runAll()
 
@@ -904,8 +904,8 @@ class TestCraftPublishingJob(TestCaseWithFactory):
             lambda self: "https://example.com/repo.git";,
         )
 
-        # Preserve RLIMIT_AS resource limit because runAll changes it which
-        # causes the whole test worker process to have limited memory
+        # Preserve the virtual memory resource limit because runAll changes it
+        # which causes the whole test worker process to have limited memory
         with preserve_rlimit(RLIMIT_AS):
             JobRunner([job]).runAll()
 
@@ -950,8 +950,8 @@ class TestCraftPublishingJob(TestCaseWithFactory):
             lambda self: "https://example.com/repo.git";,
         )
 
-        # Preserve RLIMIT_AS resource limit because runAll changes it which
-        # causes the whole test worker process to have limited memory
+        # Preserve the virtual memory resource limit because runAll changes it
+        # which causes the whole test worker process to have limited memory
         with preserve_rlimit(RLIMIT_AS):
             JobRunner([job]).runAll()
 
diff --git a/lib/lp/services/osutils.py b/lib/lp/services/osutils.py
index 3234842..d79529e 100644
--- a/lib/lp/services/osutils.py
+++ b/lib/lp/services/osutils.py
@@ -190,15 +190,14 @@ def process_exists(pid):
 
 @contextmanager
 def preserve_rlimit(resource_type: int):
-    """
-    Context manager to preserve and restore a specific resource limit.
-    """
-    saved_limits: Tuple[int, int] = getrlimit(resource_type)
+    """Context manager to preserve and restore a specific resource limit."""
+    current_limits: Tuple[int, int] = getrlimit(resource_type)
     try:
         yield
     finally:
         try:
-            setrlimit(resource_type, saved_limits)
-        except Exception:
-            # Ignore restoration errors
+            setrlimit(resource_type, current_limits)
+        except ValueError:
+            # throws a ValueError if the resource_type has no enum counterpart
+            # inside the "resource" package such as "RLIMIT_AS"
             pass