-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upload task worker results to Backend API #22
Conversation
New changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this.
Please check and maybe apply the minor wording suggestions and merge. We'll deploy it.
|
||
|
||
def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]): | ||
def get_country_data(*country_codes: str) -> dict[str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a weird API. Any reason to change from single list[str]
?
|
||
|
||
def create_worker(worker_id: str, private_key_data: bytes, country_codes: list[str]): | ||
def get_country_data(*country_codes: str) -> dict[str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not clear enough. Maybe renaming to initial_country_codes
would help
def update_worker(worker_id: str, country_codes: list[str]): | ||
"""Update worker's data. | ||
|
||
Updates the ountries for a worker to run tests from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Makes sense.
dev/docker-compose.yaml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- TEST_FILE_PATH=/zim/wikipedia/wikipedia_guw_all_nopic_2023-02.zim | |
- TEST_FILE_PATH=/zim/wikipedia/speedtest_en_blob-mini_2024-05.zim |
Rationale
Upload results from task worker to Backend API
Changes
sys.exit
calls from cli modules toentrypoint.py
Location
model to represent available test locationscountry_code
inTest
model now means the test location and not necessarily the country the mirror is locatedmirror_url
toTest
which manager uses to build test file URL for the mirrorupdate-locations
sub-command to update the list of test locationstask-worker
with output of healthcheck (which contains ip data)