launchpad-reviewers team mailing list archive

[Ines Almeida <mp+474938@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-call-remove-resources-endpoint into launchpad:master.

Commit message:
Call fetch service endpoint to remove resources at the end of a successfull build

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/474938

Ran all fetch service related tests, and all tests from test_snapbuildbehaviour, test_rockrecipebuildbehaviour and test_craftrecipebuildbehaviour: All ran successfully.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-call-remove-resources-endpoint into launchpad:master.

diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index a52ce06..7c5b3e0 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -25,6 +25,7 @@ from twisted.internet import defer
 
 from lp.buildmaster.downloader import (
     EndFetchServiceSessionCommand,
+    RemoveResourcesFetchServiceSessionCommand,
     RequestFetchServiceSessionCommand,
     RequestProxyTokenCommand,
     RetrieveFetchServiceSessionCommand,
@@ -250,6 +251,7 @@ class FetchService(IProxyService):
     TOKEN_REVOCATION = "{control_endpoint}/session/{session_id}/token"
     RETRIEVE_METADATA_ENDPOINT = "{control_endpoint}/session/{session_id}"
     END_SESSION_ENDPOINT = "{control_endpoint}/session/{session_id}"
+    REMOVE_RESOURCES_ENDPOINT = "{control_endpoint}/resources/{session_id}"
 
     def __init__(self, worker: BuilderWorker):
         self.control_endpoint = _get_value_from_config(
@@ -348,14 +350,27 @@ class FetchService(IProxyService):
     def endSession(self, session_id: str):
         """End the proxy session and do any cleanup needed.
 
+        After the end of the session, we remove the resources from the
+        successfull build given all data was already fetched by then.
+
         :raises: RequestException if request to Fetch Service fails
         """
-        url = self.END_SESSION_ENDPOINT.format(
+        end_session_url = self.END_SESSION_ENDPOINT.format(
             control_endpoint=self.control_endpoint,
             session_id=session_id,
         )
         yield self.worker.process_pool.doWork(
             EndFetchServiceSessionCommand,
-            url=url,
+            url=end_session_url,
+            auth_header=self.auth_header,
+        )
+
+        remove_resources_url = self.REMOVE_RESOURCES_ENDPOINT.format(
+            control_endpoint=self.control_endpoint,
+            session_id=session_id,
+        )
+        yield self.worker.process_pool.doWork(
+            RemoveResourcesFetchServiceSessionCommand,
+            url=remove_resources_url,
             auth_header=self.auth_header,
         )
diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
index ca256d7..d018baa 100644
--- a/lib/lp/buildmaster/downloader.py
+++ b/lib/lp/buildmaster/downloader.py
@@ -10,6 +10,7 @@ anything from the rest of Launchpad.
 __all__ = [
     "DownloadCommand",
     "EndFetchServiceSessionCommand",
+    "RemoveResourcesFetchServiceSessionCommand",
     "RequestFetchServiceSessionCommand",
     "RequestProcess",
     "RequestProxyTokenCommand",
@@ -97,6 +98,23 @@ class EndFetchServiceSessionCommand(amp.Command):
     }
 
 
+class RemoveResourcesFetchServiceSessionCommand(amp.Command):
+    """Fetch service API Command subclass to remove resources from a session.
+
+    It defines arguments and error conditions. For reference:
+    https://docs.twisted.org/en/twisted-18.7.0/core/howto/amp.html
+    """
+
+    arguments = [
+        (b"url", amp.Unicode()),
+        (b"auth_header", amp.String()),
+    ]
+    response: List[Tuple[bytes, amp.Argument]] = []
+    errors = {
+        RequestException: b"REQUEST_ERROR",
+    }
+
+
 class RequestProxyTokenCommand(amp.Command):
     arguments = [
         (b"url", amp.Unicode()),
@@ -207,3 +225,14 @@ class RequestProcess(AMPChild):
             )
             response.raise_for_status()
             return {}
+
+    @RemoveResourcesFetchServiceSessionCommand.responder
+    def removeResourcesFetchServiceSessionCommand(self, url, auth_header):
+        with Session() as session:
+            session.trust_env = False
+            response = session.delete(
+                url,
+                headers={"Authorization": auth_header},
+            )
+            response.raise_for_status()
+            return {}
diff --git a/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py b/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
index 83ebd8f..2998311 100644
--- a/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
+++ b/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
@@ -765,7 +765,8 @@ class TestAsyncCraftRecipeBuildBehaviourFetchService(
     @defer.inlineCallbacks
     def test_endProxySession(self):
         """By ending a fetch service session, metadata is retrieved from the
-        fetch service and saved to a file; and call to end the session is made.
+        fetch service and saved to a file; and call to end the session and
+        removeing resources are made.
         """
         tem_upload_path = self.useFixture(TempDir()).path
 
@@ -789,8 +790,8 @@ class TestAsyncCraftRecipeBuildBehaviourFetchService(
         # End the session
         yield job.endProxySession(upload_path=tem_upload_path)
 
-        # We expect 3 calls made to the fetch service API, in this order
-        self.assertEqual(3, len(self.fetch_service_api.sessions.requests))
+        # We expect 4 calls made to the fetch service API, in this order
+        self.assertEqual(4, len(self.fetch_service_api.sessions.requests))
 
         # Request start a session
         start_session_request = self.fetch_service_api.sessions.requests[0]
@@ -811,6 +812,14 @@ class TestAsyncCraftRecipeBuildBehaviourFetchService(
             f"/session/{session_id}".encode(), end_session_request["uri"]
         )
 
+        # Request removal of resources
+        remove_resources_request = self.fetch_service_api.sessions.requests[3]
+        self.assertEqual(b"DELETE", remove_resources_request["method"])
+        self.assertEqual(
+            f"/resources/{session_id}".encode(),
+            remove_resources_request["uri"],
+        )
+
         # The expected file is created in the `tem_upload_path`
         expected_filename = f"{job.build.build_cookie}_metadata.json"
         expected_file_path = os.path.join(tem_upload_path, expected_filename)
diff --git a/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py b/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
index 9fdffd8..5c5ed1a 100644
--- a/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
+++ b/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
@@ -766,7 +766,8 @@ class TestAsyncRockRecipeBuildBehaviourFetchService(
     @defer.inlineCallbacks
     def test_endProxySession(self):
         """By ending a fetch service session, metadata is retrieved from the
-        fetch service and saved to a file; and call to end the session is made.
+        fetch service and saved to a file; and call to end the session and
+        removeing resources are made.
         """
         tem_upload_path = self.useFixture(TempDir()).path
 
@@ -790,8 +791,8 @@ class TestAsyncRockRecipeBuildBehaviourFetchService(
         # End the session
         yield job.endProxySession(upload_path=tem_upload_path)
 
-        # We expect 3 calls made to the fetch service API, in this order
-        self.assertEqual(3, len(self.fetch_service_api.sessions.requests))
+        # We expect 4 calls made to the fetch service API, in this order
+        self.assertEqual(4, len(self.fetch_service_api.sessions.requests))
 
         # Request start a session
         start_session_request = self.fetch_service_api.sessions.requests[0]
@@ -812,6 +813,14 @@ class TestAsyncRockRecipeBuildBehaviourFetchService(
             f"/session/{session_id}".encode(), end_session_request["uri"]
         )
 
+        # Request removal of resources
+        remove_resources_request = self.fetch_service_api.sessions.requests[3]
+        self.assertEqual(b"DELETE", remove_resources_request["method"])
+        self.assertEqual(
+            f"/resources/{session_id}".encode(),
+            remove_resources_request["uri"],
+        )
+
         # The expected file is created in the `tem_upload_path`
         expected_filename = f"{job.build.build_cookie}_metadata.json"
         expected_file_path = os.path.join(tem_upload_path, expected_filename)
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 66fb638..e7204d3 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -460,7 +460,8 @@ class TestAsyncSnapBuildBehaviourFetchService(
     @defer.inlineCallbacks
     def test_endProxySession(self):
         """By ending a fetch service session, metadata is retrieved from the
-        fetch service and saved to a file; and call to end the session is made.
+        fetch service and saved to a file; and call to end the session and
+        removeing resources are made.
         """
         self.useFixture(
             FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
@@ -489,8 +490,8 @@ class TestAsyncSnapBuildBehaviourFetchService(
         # End the session
         yield job.endProxySession(upload_path=tem_upload_path)
 
-        # We expect 3 calls made to the fetch service API, in this order
-        self.assertEqual(3, len(self.fetch_service_api.sessions.requests))
+        # We expect 4 calls made to the fetch service API, in this order
+        self.assertEqual(4, len(self.fetch_service_api.sessions.requests))
 
         # Request start a session
         start_session_request = self.fetch_service_api.sessions.requests[0]
@@ -511,6 +512,14 @@ class TestAsyncSnapBuildBehaviourFetchService(
             f"/session/{session_id}".encode(), end_session_request["uri"]
         )
 
+        # Request removal of resources
+        remove_resources_request = self.fetch_service_api.sessions.requests[3]
+        self.assertEqual(b"DELETE", remove_resources_request["method"])
+        self.assertEqual(
+            f"/resources/{session_id}".encode(),
+            remove_resources_request["uri"],
+        )
+
         # The expected file is created in the `tem_upload_path`
         expected_filename = f"{job.build.build_cookie}_metadata.json"
         expected_file_path = os.path.join(tem_upload_path, expected_filename)