From 4aa10cfe35b295a6837d1f3d2bf7874121e75372 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 16 Nov 2021 11:52:20 +0100 Subject: [PATCH 01/10] Add admin API to run background jobs --- docs/sample_config.yaml | 4 +- .../admin_api/background_updates.md | 35 ++++- docs/user_directory.md | 3 +- synapse/config/user_directory.py | 4 +- synapse/rest/admin/__init__.py | 2 + synapse/rest/admin/background_updates.py | 110 ++++++++++++--- synapse/storage/background_updates.py | 2 + tests/rest/admin/test_background_updates.py | 132 ++++++++++++++++-- 8 files changed, 256 insertions(+), 36 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index d48c08f1d95f..62e1ff6bfd6b 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -2360,8 +2360,8 @@ user_directory: # indexes were (re)built was before Synapse 1.44, you'll have to # rebuild the indexes in order to search through all known users. # These indexes are built the first time Synapse starts; admins can - # manually trigger a rebuild following the instructions at - # https://matrix-org.github.io/synapse/latest/user_directory.html + # manually trigger a rebuild via API following the instructions at + # https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/background_updates.html#run # # Uncomment to return search results containing all known users, even if that # user does not share a room with the requester. diff --git a/docs/usage/administration/admin_api/background_updates.md b/docs/usage/administration/admin_api/background_updates.md index b36d7fe39867..566ba7646620 100644 --- a/docs/usage/administration/admin_api/background_updates.md +++ b/docs/usage/administration/admin_api/background_updates.md @@ -42,7 +42,6 @@ For each update: `average_items_per_ms` how many items are processed per millisecond based on an exponential average. - ## Enabled This API allow pausing background updates. @@ -82,3 +81,37 @@ The API returns the `enabled` param. ``` There is also a `GET` version which returns the `enabled` state. + + +## Run + +This API runs specific background updates. The job starts immediately after calling the API. + + +The API is: + +``` +POST /_synapse/admin/v1/background_updates/start_job +``` + +with the following body: + +```json +{ + "job_name": "populate_stats_process_rooms" +} +``` + +The API returns the `job_name` param. + +```json +{ + "job_name": "populate_stats_process_rooms" +} +``` + +The following JSON body parameters are available: + +- `job_name` - A string which job to run. Valid values are: + - `populate_stats_process_rooms` - Recalculate the stats for all rooms. + - `regenerate_directory` - Recalculate the [user directory](../../../user_directory.md) if it is stale or out of sync. diff --git a/docs/user_directory.md b/docs/user_directory.md index 07fe95489133..cb33e9d0e918 100644 --- a/docs/user_directory.md +++ b/docs/user_directory.md @@ -7,7 +7,8 @@ who are present in a publicly viewable room present on the server. The directory info is stored in various tables, which can (typically after DB corruption) get stale or out of sync. If this happens, for now the -solution to fix it is to execute the SQL [here](https://github.com/matrix-org/synapse/blob/master/synapse/storage/schema/main/delta/53/user_dir_populate.sql) +solution to fix it is to use the [admin API](usage/administration/admin_api/background_updates.md#run) or +execute the SQL [here](https://github.com/matrix-org/synapse/blob/master/synapse/storage/schema/main/delta/53/user_dir_populate.sql) and then restart synapse. This should then start a background task to flush the current tables and regenerate the directory. diff --git a/synapse/config/user_directory.py b/synapse/config/user_directory.py index 2552f688d050..6d6678c7e441 100644 --- a/synapse/config/user_directory.py +++ b/synapse/config/user_directory.py @@ -53,8 +53,8 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # indexes were (re)built was before Synapse 1.44, you'll have to # rebuild the indexes in order to search through all known users. # These indexes are built the first time Synapse starts; admins can - # manually trigger a rebuild following the instructions at - # https://matrix-org.github.io/synapse/latest/user_directory.html + # manually trigger a rebuild via API following the instructions at + # https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/background_updates.html#run # # Uncomment to return search results containing all known users, even if that # user does not share a room with the requester. diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index d78fe406c40a..e7b7decb935a 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -28,6 +28,7 @@ from synapse.rest.admin.background_updates import ( BackgroundUpdateEnabledRestServlet, BackgroundUpdateRestServlet, + BackgroundUpdateStartJobRestServlet, ) from synapse.rest.admin.devices import ( DeleteDevicesRestServlet, @@ -259,6 +260,7 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: SendServerNoticeServlet(hs).register(http_server) BackgroundUpdateEnabledRestServlet(hs).register(http_server) BackgroundUpdateRestServlet(hs).register(http_server) + BackgroundUpdateStartJobRestServlet(hs).register(http_server) def register_servlets_for_client_rest_resource( diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index 0d0183bf2086..0754f5086ecc 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -12,10 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from http import HTTPStatus from typing import TYPE_CHECKING, Tuple from synapse.api.errors import SynapseError -from synapse.http.servlet import RestServlet, parse_json_object_from_request +from synapse.http.servlet import RestServlet, assert_params_in_dict, parse_json_object_from_request from synapse.http.site import SynapseRequest from synapse.rest.admin._base import admin_patterns, assert_user_is_admin from synapse.types import JsonDict @@ -29,37 +30,38 @@ class BackgroundUpdateEnabledRestServlet(RestServlet): """Allows temporarily disabling background updates""" - PATTERNS = admin_patterns("/background_updates/enabled") + PATTERNS = admin_patterns("/background_updates/enabled$") def __init__(self, hs: "HomeServer"): - self.group_server = hs.get_groups_server_handler() - self.is_mine_id = hs.is_mine_id - self.auth = hs.get_auth() + self._is_mine_id = hs.is_mine_id + self._auth = hs.get_auth() - self.data_stores = hs.get_datastores() + self._data_stores = hs.get_datastores() async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - await assert_user_is_admin(self.auth, requester.user) + requester = await self._auth.get_user_by_req(request) + await assert_user_is_admin(self._auth, requester.user) # We need to check that all configured databases have updates enabled. # (They *should* all be in sync.) - enabled = all(db.updates.enabled for db in self.data_stores.databases) + enabled = all(db.updates.enabled for db in self._data_stores.databases) return 200, {"enabled": enabled} async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - await assert_user_is_admin(self.auth, requester.user) + requester = await self._auth.get_user_by_req(request) + await assert_user_is_admin(self._auth, requester.user) body = parse_json_object_from_request(request) enabled = body.get("enabled", True) if not isinstance(enabled, bool): - raise SynapseError(400, "'enabled' parameter must be a boolean") + raise SynapseError( + HTTPStatus.BAD_REQUEST, "'enabled' parameter must be a boolean" + ) - for db in self.data_stores.databases: + for db in self._data_stores.databases: db.updates.enabled = enabled # If we're re-enabling them ensure that we start the background @@ -73,26 +75,25 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: class BackgroundUpdateRestServlet(RestServlet): """Fetch information about background updates""" - PATTERNS = admin_patterns("/background_updates/status") + PATTERNS = admin_patterns("/background_updates/status$") def __init__(self, hs: "HomeServer"): - self.group_server = hs.get_groups_server_handler() - self.is_mine_id = hs.is_mine_id - self.auth = hs.get_auth() + self._is_mine_id = hs.is_mine_id + self._auth = hs.get_auth() - self.data_stores = hs.get_datastores() + self._data_stores = hs.get_datastores() async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - await assert_user_is_admin(self.auth, requester.user) + requester = await self._auth.get_user_by_req(request) + await assert_user_is_admin(self._auth, requester.user) # We need to check that all configured databases have updates enabled. # (They *should* all be in sync.) - enabled = all(db.updates.enabled for db in self.data_stores.databases) + enabled = all(db.updates.enabled for db in self._data_stores.databases) current_updates = {} - for db in self.data_stores.databases: + for db in self._data_stores.databases: update = db.updates.get_current_update() if not update: continue @@ -105,3 +106,68 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: } return 200, {"enabled": enabled, "current_updates": current_updates} + + +class BackgroundUpdateStartJobRestServlet(RestServlet): + """Allows to start specific background updates""" + + PATTERNS = admin_patterns("/background_updates/start_job") + + def __init__(self, hs: "HomeServer"): + self._is_mine_id = hs.is_mine_id + self._auth = hs.get_auth() + + self._store = hs.get_datastore() + + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + requester = await self._auth.get_user_by_req(request) + await assert_user_is_admin(self._auth, requester.user) + + body = parse_json_object_from_request(request) + assert_params_in_dict(body, ["job_name"]) + + job_name = body.get("job_name") + + if job_name == "populate_stats_process_rooms": + await self._store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_stats_process_rooms", + "progress_json": "{}", + }, + desc="admin_api_populate_stats_process_rooms", + ) + elif job_name == "regenerate_directory": + jobs = [ + { + "update_name": "populate_user_directory_createtables", + "progress_json": "{}", + "depends_on": None, + }, + { + "update_name": "populate_user_directory_process_rooms", + "progress_json": "{}", + "depends_on": "populate_user_directory_createtables", + }, + { + "update_name": "populate_user_directory_process_users", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_rooms", + }, + { + "update_name": "populate_user_directory_cleanup", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_users", + }, + ] + await self._store.db_pool.simple_insert_many( + table="background_updates", + values=jobs, + desc="admin_api_regenerate_directory", + ) + else: + raise SynapseError(400, "Invalid job_name") + + self._store.db_pool.updates.start_doing_background_updates() + + return 200, {"job_name": job_name} diff --git a/synapse/storage/background_updates.py b/synapse/storage/background_updates.py index b9a8ca997e61..1bab431ca6a0 100644 --- a/synapse/storage/background_updates.py +++ b/synapse/storage/background_updates.py @@ -122,6 +122,8 @@ def get_current_update(self) -> Optional[BackgroundUpdatePerformance]: def start_doing_background_updates(self) -> None: if self.enabled: + # if we start a new background update, not all updates ar done + self._all_done = False run_as_background_process("background_updates", self.run_background_updates) async def run_background_updates(self, sleep: bool = True) -> None: diff --git a/tests/rest/admin/test_background_updates.py b/tests/rest/admin/test_background_updates.py index 78c48db552de..3d0c8e52cadb 100644 --- a/tests/rest/admin/test_background_updates.py +++ b/tests/rest/admin/test_background_updates.py @@ -11,8 +11,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from http import HTTPStatus +from typing import Collection + +from parameterized import parameterized import synapse.rest.admin +from synapse.api.errors import Codes from synapse.rest.client import login from synapse.server import HomeServer @@ -30,6 +35,60 @@ def prepare(self, reactor, clock, hs: HomeServer): self.admin_user = self.register_user("admin", "pass", admin=True) self.admin_user_tok = self.login("admin", "pass") + @parameterized.expand( + [ + ("GET", "/_synapse/admin/v1/background_updates/enabled"), + ("POST", "/_synapse/admin/v1/background_updates/enabled"), + ("GET", "/_synapse/admin/v1/background_updates/status"), + ("POST", "/_synapse/admin/v1/background_updates/start_job"), + ] + ) + def test_requester_is_no_admin(self, method: str, url: str): + """ + If the user is not a server admin, an error 403 is returned. + """ + + self.register_user("user", "pass", admin=False) + other_user_tok = self.login("user", "pass") + + channel = self.make_request( + method, + url, + content={}, + access_token=other_user_tok, + ) + + self.assertEqual(HTTPStatus.FORBIDDEN, channel.code, msg=channel.json_body) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + def test_invalid_parameter(self): + """ + If parameters are invalid, an error is returned. + """ + url = "/_synapse/admin/v1/background_updates/start_job" + + # empty content + channel = self.make_request( + "POST", + url, + content={}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) + self.assertEqual(Codes.MISSING_PARAM, channel.json_body["errcode"]) + + # job_name invalid + channel = self.make_request( + "POST", + url, + content={"job_name": "unknown"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) + self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) + def _register_bg_update(self): "Adds a bg update but doesn't start it" @@ -60,7 +119,7 @@ def test_status_empty(self): "/_synapse/admin/v1/background_updates/status", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) # Background updates should be enabled, but none should be running. self.assertDictEqual( @@ -82,7 +141,7 @@ def test_status_bg_update(self): "/_synapse/admin/v1/background_updates/status", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) # Background updates should be enabled, and one should be running. self.assertDictEqual( @@ -114,7 +173,7 @@ def test_enabled(self): "/_synapse/admin/v1/background_updates/enabled", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertDictEqual(channel.json_body, {"enabled": True}) # Disable the BG updates @@ -124,7 +183,7 @@ def test_enabled(self): content={"enabled": False}, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertDictEqual(channel.json_body, {"enabled": False}) # Advance a bit and get the current status, note this will finish the in @@ -137,7 +196,7 @@ def test_enabled(self): "/_synapse/admin/v1/background_updates/status", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertDictEqual( channel.json_body, { @@ -162,7 +221,7 @@ def test_enabled(self): "/_synapse/admin/v1/background_updates/status", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) # There should be no change from the previous /status response. self.assertDictEqual( @@ -188,7 +247,7 @@ def test_enabled(self): content={"enabled": True}, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertDictEqual(channel.json_body, {"enabled": True}) @@ -199,7 +258,7 @@ def test_enabled(self): "/_synapse/admin/v1/background_updates/status", access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) # Background updates should be enabled and making progress. self.assertDictEqual( @@ -216,3 +275,60 @@ def test_enabled(self): "enabled": True, }, ) + + @parameterized.expand( + [ + ("populate_stats_process_rooms", ["populate_stats_process_rooms"]), + ( + "regenerate_directory", + [ + "populate_user_directory_createtables", + "populate_user_directory_process_rooms", + "populate_user_directory_process_users", + "populate_user_directory_cleanup", + ], + ), + ] + ) + def test_start_backround_job(self, job_name: str, updates: Collection[str]): + """ + Test that background updates add to database and be processed. + + Args: + job_name: name of the job to call with API + updates: collection of background updates to be started + """ + + # no background update is waiting + self.assertTrue( + self.get_success( + self.store.db_pool.updates.has_completed_background_updates() + ) + ) + + channel = self.make_request( + "POST", + "/_synapse/admin/v1/background_updates/start_job", + content={"job_name": job_name}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) + self.assertEqual(job_name, channel.json_body["job_name"]) + + # test that each background update is waiting now + for update in updates: + self.assertFalse( + self.get_success( + self.store.db_pool.updates.has_completed_background_update(update) + ) + ) + + self.wait_for_background_updates() + + # background updates are done + self.assertTrue( + self.get_success( + self.store.db_pool.updates.has_completed_background_updates() + ) + ) From 6e82bbd578f0b4f994cadb6dfad1d402b10da409 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 16 Nov 2021 11:58:17 +0100 Subject: [PATCH 02/10] newsfile --- changelog.d/11352.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11352.feature diff --git a/changelog.d/11352.feature b/changelog.d/11352.feature new file mode 100644 index 000000000000..a4d01b3549c1 --- /dev/null +++ b/changelog.d/11352.feature @@ -0,0 +1 @@ +Add admin API to run background jobs. \ No newline at end of file From 1f30f58143659b474ca6f0de742128ef681bce0c Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 16 Nov 2021 12:01:44 +0100 Subject: [PATCH 03/10] Fix isort --- synapse/rest/admin/background_updates.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index 0754f5086ecc..d038b4a95f2f 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -16,7 +16,11 @@ from typing import TYPE_CHECKING, Tuple from synapse.api.errors import SynapseError -from synapse.http.servlet import RestServlet, assert_params_in_dict, parse_json_object_from_request +from synapse.http.servlet import ( + RestServlet, + assert_params_in_dict, + parse_json_object_from_request, +) from synapse.http.site import SynapseRequest from synapse.rest.admin._base import admin_patterns, assert_user_is_admin from synapse.types import JsonDict From f3ee190f6fe19681c22997e912108f929974ae73 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 16 Nov 2021 12:40:14 +0100 Subject: [PATCH 04/10] mypy --- synapse/rest/admin/background_updates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index d038b4a95f2f..d54ef401d54b 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -146,7 +146,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: { "update_name": "populate_user_directory_createtables", "progress_json": "{}", - "depends_on": None, + "depends_on": "", }, { "update_name": "populate_user_directory_process_rooms", From 817fa21472e8f63076236da771dd08a051754b23 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Nov 2021 19:37:48 +0100 Subject: [PATCH 05/10] Apply suggestions from code review Co-authored-by: Patrick Cloke --- docs/usage/administration/admin_api/background_updates.md | 2 +- synapse/rest/admin/background_updates.py | 2 +- synapse/storage/background_updates.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/usage/administration/admin_api/background_updates.md b/docs/usage/administration/admin_api/background_updates.md index 566ba7646620..1d74bc24c2b5 100644 --- a/docs/usage/administration/admin_api/background_updates.md +++ b/docs/usage/administration/admin_api/background_updates.md @@ -85,7 +85,7 @@ There is also a `GET` version which returns the `enabled` state. ## Run -This API runs specific background updates. The job starts immediately after calling the API. +This API schedules a specific background update to run. The job starts immediately after calling the API. The API is: diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index d54ef401d54b..a53f96087e66 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -130,7 +130,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: body = parse_json_object_from_request(request) assert_params_in_dict(body, ["job_name"]) - job_name = body.get("job_name") + job_name = body["job_name"] if job_name == "populate_stats_process_rooms": await self._store.db_pool.simple_insert( diff --git a/synapse/storage/background_updates.py b/synapse/storage/background_updates.py index 1bab431ca6a0..b104f9032cf0 100644 --- a/synapse/storage/background_updates.py +++ b/synapse/storage/background_updates.py @@ -122,7 +122,7 @@ def get_current_update(self) -> Optional[BackgroundUpdatePerformance]: def start_doing_background_updates(self) -> None: if self.enabled: - # if we start a new background update, not all updates ar done + # if we start a new background update, not all updates are done. self._all_done = False run_as_background_process("background_updates", self.run_background_updates) From b52668732706136423b4197730b0a46b61ffe575 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Nov 2021 20:58:18 +0100 Subject: [PATCH 06/10] change `int` to `HTTPStatus` and remove not needed `_is_mind_id` --- synapse/rest/admin/background_updates.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index a53f96087e66..4ad5123dadd2 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -37,9 +37,7 @@ class BackgroundUpdateEnabledRestServlet(RestServlet): PATTERNS = admin_patterns("/background_updates/enabled$") def __init__(self, hs: "HomeServer"): - self._is_mine_id = hs.is_mine_id self._auth = hs.get_auth() - self._data_stores = hs.get_datastores() async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -50,7 +48,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # (They *should* all be in sync.) enabled = all(db.updates.enabled for db in self._data_stores.databases) - return 200, {"enabled": enabled} + return HTTPStatus.OK, {"enabled": enabled} async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self._auth.get_user_by_req(request) @@ -73,7 +71,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if enabled: db.updates.start_doing_background_updates() - return 200, {"enabled": enabled} + return HTTPStatus.OK, {"enabled": enabled} class BackgroundUpdateRestServlet(RestServlet): @@ -82,9 +80,7 @@ class BackgroundUpdateRestServlet(RestServlet): PATTERNS = admin_patterns("/background_updates/status$") def __init__(self, hs: "HomeServer"): - self._is_mine_id = hs.is_mine_id self._auth = hs.get_auth() - self._data_stores = hs.get_datastores() async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -109,7 +105,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "average_items_per_ms": update.average_items_per_ms(), } - return 200, {"enabled": enabled, "current_updates": current_updates} + return HTTPStatus.OK, {"enabled": enabled, "current_updates": current_updates} class BackgroundUpdateStartJobRestServlet(RestServlet): @@ -118,9 +114,7 @@ class BackgroundUpdateStartJobRestServlet(RestServlet): PATTERNS = admin_patterns("/background_updates/start_job") def __init__(self, hs: "HomeServer"): - self._is_mine_id = hs.is_mine_id self._auth = hs.get_auth() - self._store = hs.get_datastore() async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -170,8 +164,8 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: desc="admin_api_regenerate_directory", ) else: - raise SynapseError(400, "Invalid job_name") + raise SynapseError(HTTPStatus.BAD_REQUEST, "Invalid job_name") self._store.db_pool.updates.start_doing_background_updates() - return 200, {"job_name": job_name} + return HTTPStatus.OK, {"job_name": job_name} From 093d0f5a21dcb0388ca3d11f2bdb14383c4d92b1 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Nov 2021 21:08:15 +0100 Subject: [PATCH 07/10] update doc and remove return value --- docs/usage/administration/admin_api/background_updates.md | 8 -------- docs/user_directory.md | 7 +++---- synapse/rest/admin/background_updates.py | 2 +- tests/rest/admin/test_background_updates.py | 1 - 4 files changed, 4 insertions(+), 14 deletions(-) diff --git a/docs/usage/administration/admin_api/background_updates.md b/docs/usage/administration/admin_api/background_updates.md index 1d74bc24c2b5..9f6ac7d56784 100644 --- a/docs/usage/administration/admin_api/background_updates.md +++ b/docs/usage/administration/admin_api/background_updates.md @@ -102,14 +102,6 @@ with the following body: } ``` -The API returns the `job_name` param. - -```json -{ - "job_name": "populate_stats_process_rooms" -} -``` - The following JSON body parameters are available: - `job_name` - A string which job to run. Valid values are: diff --git a/docs/user_directory.md b/docs/user_directory.md index cb33e9d0e918..c4794b04cf61 100644 --- a/docs/user_directory.md +++ b/docs/user_directory.md @@ -6,10 +6,9 @@ on this particular server - i.e. ones which your account shares a room with, or who are present in a publicly viewable room present on the server. The directory info is stored in various tables, which can (typically after -DB corruption) get stale or out of sync. If this happens, for now the -solution to fix it is to use the [admin API](usage/administration/admin_api/background_updates.md#run) or -execute the SQL [here](https://github.com/matrix-org/synapse/blob/master/synapse/storage/schema/main/delta/53/user_dir_populate.sql) -and then restart synapse. This should then start a background task to +DB corruption) get stale or out of sync. If this happens, for now the +solution to fix it is to use the [admin API](usage/administration/admin_api/background_updates.md#run) +and execute the job `regenerate_directory`. This should then start a background task to flush the current tables and regenerate the directory. Data model diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index 4ad5123dadd2..f1c5dd5f2ae1 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -168,4 +168,4 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: self._store.db_pool.updates.start_doing_background_updates() - return HTTPStatus.OK, {"job_name": job_name} + return HTTPStatus.OK, {} diff --git a/tests/rest/admin/test_background_updates.py b/tests/rest/admin/test_background_updates.py index 3d0c8e52cadb..fb68234df106 100644 --- a/tests/rest/admin/test_background_updates.py +++ b/tests/rest/admin/test_background_updates.py @@ -314,7 +314,6 @@ def test_start_backround_job(self, job_name: str, updates: Collection[str]): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertEqual(job_name, channel.json_body["job_name"]) # test that each background update is waiting now for update in updates: From ac4437292022e88413d373f5e0396dbf45c59f90 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Nov 2021 21:53:25 +0100 Subject: [PATCH 08/10] add handling of duplicated jobs --- synapse/rest/admin/background_updates.py | 37 +++++++++++++-------- tests/rest/admin/test_background_updates.py | 23 +++++++++++++ 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index f1c5dd5f2ae1..90c31e554e25 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -112,6 +112,7 @@ class BackgroundUpdateStartJobRestServlet(RestServlet): """Allows to start specific background updates""" PATTERNS = admin_patterns("/background_updates/start_job") + ERROR_DUPLICATE_JOB = "Job %s is already in queue of background updates." def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() @@ -127,14 +128,19 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: job_name = body["job_name"] if job_name == "populate_stats_process_rooms": - await self._store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_stats_process_rooms", - "progress_json": "{}", - }, - desc="admin_api_populate_stats_process_rooms", - ) + try: + await self._store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_stats_process_rooms", + "progress_json": "{}", + }, + desc="admin_api_populate_stats_process_rooms", + ) + except self._store.db_pool.engine.module.IntegrityError: + raise SynapseError( + HTTPStatus.BAD_REQUEST, self.ERROR_DUPLICATE_JOB % job_name + ) elif job_name == "regenerate_directory": jobs = [ { @@ -158,11 +164,16 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "depends_on": "populate_user_directory_process_users", }, ] - await self._store.db_pool.simple_insert_many( - table="background_updates", - values=jobs, - desc="admin_api_regenerate_directory", - ) + try: + await self._store.db_pool.simple_insert_many( + table="background_updates", + values=jobs, + desc="admin_api_regenerate_directory", + ) + except self._store.db_pool.engine.module.IntegrityError: + raise SynapseError( + HTTPStatus.BAD_REQUEST, self.ERROR_DUPLICATE_JOB % job_name + ) else: raise SynapseError(HTTPStatus.BAD_REQUEST, "Invalid job_name") diff --git a/tests/rest/admin/test_background_updates.py b/tests/rest/admin/test_background_updates.py index fb68234df106..1786316763c0 100644 --- a/tests/rest/admin/test_background_updates.py +++ b/tests/rest/admin/test_background_updates.py @@ -331,3 +331,26 @@ def test_start_backround_job(self, job_name: str, updates: Collection[str]): self.store.db_pool.updates.has_completed_background_updates() ) ) + + def test_start_backround_job_twice(self): + """Test that add a background update twice return an error.""" + + # add job to database + self.get_success( + self.store.db_pool.simple_insert( + table="background_updates", + values={ + "update_name": "populate_stats_process_rooms", + "progress_json": "{}", + }, + ) + ) + + channel = self.make_request( + "POST", + "/_synapse/admin/v1/background_updates/start_job", + content={"job_name": "populate_stats_process_rooms"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) From 85abce5326934fd47f5ae281d89bcf86ffe48d66 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 19 Nov 2021 17:29:04 +0100 Subject: [PATCH 09/10] rework if/else --- synapse/rest/admin/background_updates.py | 42 ++++++++++-------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index 90c31e554e25..e54448602664 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -112,7 +112,6 @@ class BackgroundUpdateStartJobRestServlet(RestServlet): """Allows to start specific background updates""" PATTERNS = admin_patterns("/background_updates/start_job") - ERROR_DUPLICATE_JOB = "Job %s is already in queue of background updates." def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() @@ -128,19 +127,12 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: job_name = body["job_name"] if job_name == "populate_stats_process_rooms": - try: - await self._store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_stats_process_rooms", - "progress_json": "{}", - }, - desc="admin_api_populate_stats_process_rooms", - ) - except self._store.db_pool.engine.module.IntegrityError: - raise SynapseError( - HTTPStatus.BAD_REQUEST, self.ERROR_DUPLICATE_JOB % job_name - ) + jobs = [ + { + "update_name": "populate_stats_process_rooms", + "progress_json": "{}", + }, + ] elif job_name == "regenerate_directory": jobs = [ { @@ -164,19 +156,21 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "depends_on": "populate_user_directory_process_users", }, ] - try: - await self._store.db_pool.simple_insert_many( - table="background_updates", - values=jobs, - desc="admin_api_regenerate_directory", - ) - except self._store.db_pool.engine.module.IntegrityError: - raise SynapseError( - HTTPStatus.BAD_REQUEST, self.ERROR_DUPLICATE_JOB % job_name - ) else: raise SynapseError(HTTPStatus.BAD_REQUEST, "Invalid job_name") + try: + await self._store.db_pool.simple_insert_many( + table="background_updates", + values=jobs, + desc=f"admin_api_run_{job_name}", + ) + except self._store.db_pool.engine.module.IntegrityError: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Job %s is already in queue of background updates." % job_name, + ) + self._store.db_pool.updates.start_doing_background_updates() return HTTPStatus.OK, {} From ae88c9b83f4fc86164ea57349b3c7e3c7c7c48ce Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 13:21:28 -0500 Subject: [PATCH 10/10] Format via tuple. --- synapse/rest/admin/background_updates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/admin/background_updates.py b/synapse/rest/admin/background_updates.py index e54448602664..479672d4d568 100644 --- a/synapse/rest/admin/background_updates.py +++ b/synapse/rest/admin/background_updates.py @@ -168,7 +168,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: except self._store.db_pool.engine.module.IntegrityError: raise SynapseError( HTTPStatus.BAD_REQUEST, - "Job %s is already in queue of background updates." % job_name, + "Job %s is already in queue of background updates." % (job_name,), ) self._store.db_pool.updates.start_doing_background_updates()