Upload task worker results to Backend API #22

elfkuzco · 2024-07-08T16:58:14Z

Rationale

Upload results from task worker to Backend API

Changes

move sys.exit calls from cli modules to entrypoint.py
add Location model to represent available test locations
country_code in Test model now means the test location and not necessarily the country the mirror is located
add mirror_url to Test which manager uses to build test file URL for the mirror
fetch list of test locations from Mullvad API
add update-locations sub-command to update the list of test locations
scheduler checks if there are any test locations in db before starting, attempts to create if none exist
create test entries in all the available location for each mirror that a worker is responsible for.
merge metrics results from task-worker with output of healthcheck (which contains ip data)
upload results to backend API

codecov-commenter · 2024-07-08T17:16:10Z

Codecov Report

Attention: Patch coverage is 48.82629% with 109 lines in your changes missing coverage. Please review.

Project coverage is 67.79%. Comparing base (0776842) to head (ede4da7).

Files	Patch %	Lines
backend/src/mirrors_qa_backend/entrypoint.py	0.00%	33 Missing ⚠️
backend/src/mirrors_qa_backend/locations.py	0.00%	32 Missing ⚠️
backend/src/mirrors_qa_backend/cli/scheduler.py	0.00%	14 Missing ⚠️
backend/src/mirrors_qa_backend/cli/locations.py	0.00%	7 Missing ⚠️
backend/src/mirrors_qa_backend/country.py	44.44%	5 Missing ⚠️
backend/src/mirrors_qa_backend/db/mirrors.py	33.33%	4 Missing ⚠️
backend/src/mirrors_qa_backend/db/worker.py	80.00%	2 Missing and 1 partial ⚠️
backend/src/mirrors_qa_backend/cli/mirrors.py	0.00%	2 Missing ⚠️
backend/src/mirrors_qa_backend/cli/worker.py	77.77%	2 Missing ⚠️
backend/src/mirrors_qa_backend/extract.py	50.00%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   69.62%   67.79%   -1.83%     
==========================================
  Files          27       31       +4     
  Lines         711      829     +118     
  Branches       64       72       +8     
==========================================
+ Hits          495      562      +67     
- Misses        202      257      +55     
+ Partials       14       10       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elfkuzco · 2024-07-15T15:13:45Z

New changes

Set up a many-to-many relationship between worker and mirror country. Thus, a worker can be responsible for testing many mirrors in a country and a country can be tested by many workers.
When --countries argument is omitted, all available mirror countries are assigned to the worker being created/updated
fetch all the available countries from db when no country codes are supplied by the caller
add update-worker subcommand to update the list of countries a worker is responsible for

rgaudin

Thank you, this is very good. I couldn't test it due to lack of time but am looking forward to it.

That said, more docstrings would be welcome, especially in dedicated modules. It's not always clear what those functions are used for.

Now the main issue I have:

Sorry for the lack of reply on Slack ; and thank you for going through with your plan on this Location change.
I think this contradicts what we decided earlier and it removes a lot of flexibility.
If I understand correctly, the backend is now querying mullvad for a list of countries to test from.
This create a dependency to mullvad at the backend level that we don't want. We want to be able to easily switch VPN providers, use none or even combine them.
That's why we want the worker to expose its capabilities. One worker could have mullvad (our choice) and use the API or a listig of local config files to inform the backend of whats possible for it. Another worker could have no VPN and use its direct ISP to test some countries (we dont support this ATM, not a priority), another worker from someone else could use another wireguard-compatible VPN provider (we dont support this neither ATM, not prio either).
What's important is that we keep this door open by keeping worker-related details on the worker and not moving it to the backend.

With that in mind, I dont see the advantage of having a Location table. Countries for test creation would be dynamic from what's currently available (from recently-seen workers)

dev/docker-compose.yaml

backend/src/mirrors_qa_backend/extract.py

backend/src/mirrors_qa_backend/country.py

backend/src/mirrors_qa_backend/cli/scheduler.py

backend/src/mirrors_qa_backend/locations.py

worker/manager/src/mirrors_qa_manager/worker.py

worker/task/src/mirrors_qa_task/settings.py

worker/task/src/mirrors_qa_task/worker.py

worker/task/src/mirrors_qa_task/settings.py

rgaudin

Let's merge this.
Please check and maybe apply the minor wording suggestions and merge. We'll deploy it.

rgaudin · 2024-07-24T14:54:04Z

backend/src/mirrors_qa_backend/cli/worker.py



-def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]):
+def get_country_data(*country_codes: str) -> dict[str, str]:


That's a weird API. Any reason to change from single list[str] ?

rgaudin · 2024-07-24T14:54:41Z

backend/src/mirrors_qa_backend/cli/worker.py



-def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]):
+def get_country_data(*country_codes: str) -> dict[str, str]:


Suggested change

def get_country_data(*country_codes: str) -> dict[str, str]:

def get_country_mapping(*country_codes: str) -> dict[str, str]:

Clearer than data which is very vague

rgaudin · 2024-07-24T14:56:39Z

backend/src/mirrors_qa_backend/cli/worker.py

+def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]):
+    """Create a worker in the DB.
+
+    Assigns the countries for a worker to run tests from.


This is not clear enough. Maybe renaming to initial_country_codes would help

rgaudin · 2024-07-24T15:00:17Z

backend/src/mirrors_qa_backend/cli/worker.py

+def update_worker(worker_id: str, country_codes: list[str]):
+    """Update worker's data.
+
+    Updates the ountries for a worker to run tests from.


typo

You see you have a duplicated block to assign countries to a worker.
Given this is only useful for tests, maybe the create and update methods should not include it
and the cli command should call a new method after create/update instead. WDYT?

Okay. Makes sense.

rgaudin · 2024-07-24T15:14:48Z

dev/docker-compose.yaml

@@ -67,6 +69,7 @@ services:
      - BACKEND_API_URI=http://backend
      - SLEEP_DURATION=5m
      - TASK_WORKER_IMAGE=mirrors-qa-task-worker
+      - TEST_FILE_PATH=/zim/wikipedia/wikipedia_guw_all_nopic_2023-02.zim


Suggested change

- TEST_FILE_PATH=/zim/wikipedia/wikipedia_guw_all_nopic_2023-02.zim

- TEST_FILE_PATH=/zim/wikipedia/speedtest_en_blob-mini_2024-05.zim

elfkuzco added 4 commits July 8, 2024 17:31

create tests for all available locations per mirror

8532b30

upload speedtest results to backend api

6047a93

make results from speedtest conform to backend api input

d57ad1c

make all services depend on backend

245aed8

This was linked to issues Jul 8, 2024

Tester worker #11

Closed

Worker manager #8

Closed

autouse dbsession fixture to create db tables

aaa2a75

elfkuzco requested a review from rgaudin July 8, 2024 17:22

elfkuzco added 3 commits July 15, 2024 15:43

set up m2m relationship between worker and mirror country

9580ffe

add cli to update worker countries

4a9084d

document update to create-worker --countries argument

ede4da7

rgaudin requested changes Jul 15, 2024

View reviewed changes

elfkuzco added 4 commits July 17, 2024 06:39

remove locations table and allow worker to specify countries on creation

80b0b97

pass file url to task worker as cmd argument

3055d20

accept test file url as cmd argument

af271c0

update env var for test file

28a40a0

elfkuzco requested a review from rgaudin July 17, 2024 05:48

rgaudin approved these changes Jul 24, 2024

View reviewed changes

give variables more descriptive names

81c60e9

elfkuzco merged commit 8e7ad26 into main Jul 25, 2024
4 checks passed

elfkuzco deleted the task-worker branch July 25, 2024 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload task worker results to Backend API #22

Upload task worker results to Backend API #22

elfkuzco commented Jul 8, 2024

codecov-commenter commented Jul 8, 2024 •

edited

Loading

elfkuzco commented Jul 15, 2024

rgaudin left a comment

rgaudin left a comment

rgaudin Jul 24, 2024

rgaudin Jul 24, 2024

rgaudin Jul 24, 2024

rgaudin Jul 24, 2024

elfkuzco Jul 25, 2024

rgaudin Jul 24, 2024



		def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]):
		def get_country_data(*country_codes: str) -> dict[str, str]:

	def get_country_data(*country_codes: str) -> dict[str, str]:
	def get_country_mapping(*country_codes: str) -> dict[str, str]:

	- TEST_FILE_PATH=/zim/wikipedia/wikipedia_guw_all_nopic_2023-02.zim
	- TEST_FILE_PATH=/zim/wikipedia/speedtest_en_blob-mini_2024-05.zim

Upload task worker results to Backend API #22

Upload task worker results to Backend API #22

Conversation

elfkuzco commented Jul 8, 2024

Rationale

Changes

codecov-commenter commented Jul 8, 2024 • edited Loading

Codecov Report

elfkuzco commented Jul 15, 2024

New changes

rgaudin left a comment

Choose a reason for hiding this comment

rgaudin left a comment

Choose a reason for hiding this comment

rgaudin Jul 24, 2024

Choose a reason for hiding this comment

rgaudin Jul 24, 2024

Choose a reason for hiding this comment

rgaudin Jul 24, 2024

Choose a reason for hiding this comment

rgaudin Jul 24, 2024

Choose a reason for hiding this comment

elfkuzco Jul 25, 2024

Choose a reason for hiding this comment

rgaudin Jul 24, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jul 8, 2024 •

edited

Loading