Merge pull request #685 from FlorentinD/fix-delete-paused-db

FlorentinD · web-flow · commit c0608bf00626 · 2024-07-16T12:06:40.000+02:00
Fix deleting session by name against paused instances
diff --git a/doc/modules/ROOT/pages/tutorials/gds-sessions.adoc b/doc/modules/ROOT/pages/tutorials/gds-sessions.adoc
@@ -124,12 +124,12 @@ data_query = """
     (brie:Person {name: 'Brie',   age: 31, experience: 6, hipster: 0}),
     (elsa:Person {name: 'Elsa',   age: 65, experience: 23, hipster: 1}),
     (john:Person {name: 'John',   age: 4, experience: 100, hipster: 0}),
-    
+
     (apple:Fruit {name: 'Apple',   tropical: 0, sourness: 0.3, sweetness: 0.6}),
     (banana:Fruit {name: 'Banana', tropical: 1, sourness: 0.1, sweetness: 0.9}),
     (mango:Fruit {name: 'Mango',   tropical: 1, sourness: 0.3, sweetness: 1.0}),
     (plum:Fruit {name: 'Plum',     tropical: 0, sourness: 0.5, sweetness: 0.8})
-  
+
   CREATE
     (dan)-[:LIKES]->(apple),
     (annie)-[:LIKES]->(banana),
@@ -138,7 +138,7 @@ data_query = """
     (brie)-[:LIKES]->(banana),
     (elsa)-[:LIKES]->(plum),
     (john)-[:LIKES]->(plum),
-  
+
     (dan)-[:KNOWS]->(annie),
     (dan)-[:KNOWS]->(matt),
     (annie)-[:KNOWS]->(matt),
@@ -179,16 +179,16 @@ G, result = gds.graph.project(
     CALL {
         MATCH (p1:Person)
         OPTIONAL MATCH (p1)-[r:KNOWS]->(p2:Person)
-        RETURN 
-          p1 AS source, r AS rel, p2 AS target, 
-          p1 {.age, .experience, .hipster } AS sourceNodeProperties, 
+        RETURN
+          p1 AS source, r AS rel, p2 AS target,
+          p1 {.age, .experience, .hipster } AS sourceNodeProperties,
           p2 {.age, .experience, .hipster } AS targetNodeProperties
         UNION
         MATCH (f:Fruit)
         OPTIONAL MATCH (f)<-[r:LIKES]-(p:Person)
-        RETURN 
-          p AS source, r AS rel, f AS target, 
-          p {.age, .experience, .hipster } AS sourceNodeProperties, 
+        RETURN
+          p AS source, r AS rel, f AS target,
+          p {.age, .experience, .hipster } AS sourceNodeProperties,
           f { .tropical, .sourness, .sweetness } AS targetNodeProperties
     }
     RETURN gds.graph.project.remote(source, target, {
@@ -265,8 +265,8 @@ instance.
 ----
 gds.run_cypher(
     """
-    MATCH (p:Person) 
-    RETURN p.name, p.pagerank AS rank, p.louvain 
+    MATCH (p:Person)
+    RETURN p.name, p.pagerank AS rank, p.louvain
      ORDER BY rank DESC
     """
 )
@@ -284,7 +284,9 @@ stop incurring costs.
 
 [source, python, role=no-test]
 ----
-sessions.delete("people-and-fruits")
+gds.delete()
+
+# or sessions.delete("people-and-fruits")
 ----
 
 [source, python, role=no-test]
diff --git a/examples/gds-sessions.ipynb b/examples/gds-sessions.ipynb
@@ -181,12 +181,12 @@
     "    (brie:Person {name: 'Brie',   age: 31, experience: 6, hipster: 0}),\n",
     "    (elsa:Person {name: 'Elsa',   age: 65, experience: 23, hipster: 1}),\n",
     "    (john:Person {name: 'John',   age: 4, experience: 100, hipster: 0}),\n",
-    "    \n",
+    "\n",
     "    (apple:Fruit {name: 'Apple',   tropical: 0, sourness: 0.3, sweetness: 0.6}),\n",
     "    (banana:Fruit {name: 'Banana', tropical: 1, sourness: 0.1, sweetness: 0.9}),\n",
     "    (mango:Fruit {name: 'Mango',   tropical: 1, sourness: 0.3, sweetness: 1.0}),\n",
     "    (plum:Fruit {name: 'Plum',     tropical: 0, sourness: 0.5, sweetness: 0.8})\n",
-    "  \n",
+    "\n",
     "  CREATE\n",
     "    (dan)-[:LIKES]->(apple),\n",
     "    (annie)-[:LIKES]->(banana),\n",
@@ -195,7 +195,7 @@
     "    (brie)-[:LIKES]->(banana),\n",
     "    (elsa)-[:LIKES]->(plum),\n",
     "    (john)-[:LIKES]->(plum),\n",
-    "  \n",
+    "\n",
     "    (dan)-[:KNOWS]->(annie),\n",
     "    (dan)-[:KNOWS]->(matt),\n",
     "    (annie)-[:KNOWS]->(matt),\n",
@@ -242,16 +242,16 @@
     "    CALL {\n",
     "        MATCH (p1:Person)\n",
     "        OPTIONAL MATCH (p1)-[r:KNOWS]->(p2:Person)\n",
-    "        RETURN \n",
-    "          p1 AS source, r AS rel, p2 AS target, \n",
-    "          p1 {.age, .experience, .hipster } AS sourceNodeProperties, \n",
+    "        RETURN\n",
+    "          p1 AS source, r AS rel, p2 AS target,\n",
+    "          p1 {.age, .experience, .hipster } AS sourceNodeProperties,\n",
     "          p2 {.age, .experience, .hipster } AS targetNodeProperties\n",
     "        UNION\n",
     "        MATCH (f:Fruit)\n",
     "        OPTIONAL MATCH (f)<-[r:LIKES]-(p:Person)\n",
-    "        RETURN \n",
-    "          p AS source, r AS rel, f AS target, \n",
-    "          p {.age, .experience, .hipster } AS sourceNodeProperties, \n",
+    "        RETURN\n",
+    "          p AS source, r AS rel, f AS target,\n",
+    "          p {.age, .experience, .hipster } AS sourceNodeProperties,\n",
     "          f { .tropical, .sourness, .sweetness } AS targetNodeProperties\n",
     "    }\n",
     "    RETURN gds.graph.project.remote(source, target, {\n",
@@ -361,8 +361,8 @@
    "source": [
     "gds.run_cypher(\n",
     "    \"\"\"\n",
-    "    MATCH (p:Person) \n",
-    "    RETURN p.name, p.pagerank AS rank, p.louvain \n",
+    "    MATCH (p:Person)\n",
+    "    RETURN p.name, p.pagerank AS rank, p.louvain\n",
     "     ORDER BY rank DESC\n",
     "    \"\"\"\n",
     ")"
@@ -391,7 +391,9 @@
    },
    "outputs": [],
    "source": [
-    "sessions.delete(\"people-and-fruits\")"
+    "gds.delete()\n",
+    "\n",
+    "# or sessions.delete(\"people-and-fruits\")"
    ]
   },
   {
diff --git a/graphdatascience/session/dedicated_sessions.py b/graphdatascience/session/dedicated_sessions.py
@@ -51,7 +51,6 @@ def get_or_create(
         # hashing the password to avoid storing the actual db password in Aura
         password = hashlib.sha256(db_connection.password.encode()).hexdigest()
 
-        # TODO configure session size (and check existing_session has same size)
         if existing_session:
             self._check_expiry_date(existing_session)
             self._check_memory_configuration(existing_session, memory.value)
@@ -71,7 +70,7 @@ def get_or_create(
         )
 
         return self._construct_client(
-            session_name=session_name, session_connection=session_connection, db_connection=db_connection
+            session_id=session_id, session_connection=session_connection, db_connection=db_connection
         )
 
     def delete(self, session_name: str, dbid: Optional[str] = None) -> bool:
@@ -106,7 +105,13 @@ def list(self) -> List[SessionInfo]:
         return [SessionInfo.from_session_details(i) for i in sessions]
 
     def _find_existing_session(self, session_name: str, dbid: str) -> Optional[SessionDetails]:
-        matched_sessions = [s for s in self._aura_api.list_sessions(dbid) if s.name == session_name]
+        matched_sessions: List[SessionDetails] = []
+        try:
+            matched_sessions = [s for s in self._aura_api.list_sessions(dbid) if s.name == session_name]
+        except HTTPError as e:
+            # ignore 404 errors when listing sessions as it could mean paused sessions or deleted sessions
+            if e.response.status_code != 404:
+                raise e
 
         if len(matched_sessions) == 0:
             return None
@@ -132,12 +137,14 @@ def _create_session(
         return create_details
 
     def _construct_client(
-        self, session_name: str, session_connection: DbmsConnectionInfo, db_connection: DbmsConnectionInfo
+        self, session_id: str, session_connection: DbmsConnectionInfo, db_connection: DbmsConnectionInfo
     ) -> AuraGraphDataScience:
         return AuraGraphDataScience(
             gds_session_connection_info=session_connection,
             aura_db_connection_info=db_connection,
-            delete_fn=lambda: self.delete(session_name, dbid=AuraApi.extract_id(db_connection.uri)),
+            delete_fn=lambda: self._aura_api.delete_session(
+                session_id=session_id, dbid=AuraApi.extract_id(db_connection.uri)
+            ),
         )
 
     def _check_expiry_date(self, session: SessionDetails) -> None:
diff --git a/graphdatascience/tests/unit/test_dedicated_sessions.py b/graphdatascience/tests/unit/test_dedicated_sessions.py
@@ -69,6 +69,15 @@ def add_session(self, session: SessionDetails) -> None:
 
         self._sessions[session.id] = session
 
+    # aura behaviour of paused instances not being in an orchestra
+    def _mimic_paused_db_behaviour(self, dbid: str) -> None:
+        db = self.list_instance(dbid)
+        if db and db.status == "paused":
+            response = Response()
+            response.status_code = 404
+            response._content = b"database not found"
+            raise HTTPError(request=None, response=response)
+
     def create_instance(
         self, name: str, memory: SessionMemoryValue, cloud_provider: str, region: str
     ) -> InstanceCreateDetails:
@@ -104,20 +113,16 @@ def delete_instance(self, instance_id: str) -> InstanceSpecificDetails:
         return self._instances.pop(instance_id)
 
     def list_sessions(self, dbid: str) -> List[SessionDetails]:
-        # mimic aura behaviour of paused instances not being in an orchestra
-        db = self.list_instance(dbid)
-        if db and db.status == "paused":
-            response = Response()
-            response.status_code = 404
-            response._content = b"database not found"
-            raise HTTPError(request=None, response=response)
+        self._mimic_paused_db_behaviour(dbid)
 
         return [v for _, v in self._sessions.items() if v.instance_id == dbid]
 
     def list_instances(self) -> List[InstanceDetails]:
         return [v for _, v in self._instances.items()]
 
     def list_session(self, session_id: str, dbid: str) -> Optional[SessionDetails]:
+        self._mimic_paused_db_behaviour(dbid)
+
         matched_instance = self._sessions.get(session_id, None)
 
         if matched_instance:
@@ -239,7 +244,7 @@ def test_create_session(mocker: MockerFixture, aura_api: AuraApi) -> None:
         "session_connection": DbmsConnectionInfo(
             uri="neo4j+s://foo.bar", username="neo4j", password=HASHED_DB_PASSWORD
         ),
-        "session_name": "my-session",
+        "session_id": "ffff0-ffff1",
     }
     assert [i.name for i in sessions.list()] == ["my-session"]
 
@@ -269,7 +274,7 @@ def test_get_or_create(mocker: MockerFixture, aura_api: AuraApi) -> None:
         "session_connection": DbmsConnectionInfo(
             uri="neo4j+s://foo.bar", username="neo4j", password=HASHED_DB_PASSWORD
         ),
-        "session_name": "my-session",
+        "session_id": "ffff0-ffff1",
     }
     assert gds_args1 == gds_args2
 
@@ -399,6 +404,35 @@ def test_delete_nonunique_session(aura_api: AuraApi) -> None:
     assert len(sessions.list()) == 2
 
 
+def test_delete_session_paused_instance(aura_api: AuraApi) -> None:
+    fake_aura_api = cast(FakeAuraApi, aura_api)
+
+    fake_aura_api.id_counter += 1
+    paused_db = InstanceSpecificDetails(
+        id="4242",
+        status="paused",
+        connection_url="foo.bar",
+        memory=SessionMemory.m_16GB.value,
+        type="",
+        region="dresden",
+        name="paused-db",
+        tenant_id=fake_aura_api._tenant_id,
+        cloud_provider="aws",
+    )
+    fake_aura_api._instances[paused_db.id] = paused_db
+
+    session = aura_api.create_session(
+        name="gds-session-my-session-name",
+        dbid=paused_db.id,
+        pwd="some_pwd",
+        memory=SessionMemory.m_8GB.value,
+    )
+    sessions = DedicatedSessions(aura_api)
+
+    # cannot delete session running against a paused instance
+    assert not sessions.delete(session.name)
+
+
 def test_create_waiting_forever() -> None:
     aura_api = FakeAuraApi(status_after_creating="updating")
     _setup_db_instance(aura_api)
