Raise an exception if any indexer workers fail their health check #3726

AetherUnbound · 2024-01-31T01:32:02Z

Fixes

Description

This PR adds some exception handling to the case where the indexer workers fail their healthchecks. Those exceptions are now raised appropriately in the ingestion server and will be alerted to Slack along with other exceptions.

Testing Instructions

I added some unit tests for these cases - this is my first time using pook and I'm basing it off of other tests in the ingestion server suite, so let me know if I'm doing something that isn't idiomatic!

CI passing should be a positive indicator.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend

This LGTM. I've shared some feedback on the pook usage to give hints for how to make the tests a bit simpler and leverage pook's abilities better (namely, by adding the expected body to the matcher, rather than manually checking it after the fact), but none of them are blockers, especially as the reviews on this PR have been delayed anyway.

sarayourfriend · 2024-02-07T02:43:50Z

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py

+        for worker, mock_post, (start_id, end_id) in zip(
+            workers, post_mocks, expected_ranges
+        ):
+            assert mock_post.calls > 0


Just a suggestion to use mock_post.matched (which itself does .calls > 0), or probably better, mock_post.done to ensure it was matched the expected number of times (in this case, once). matched and calls > 0 just checks if it was matched at all, not that it was matched the expected number of times. In this case it is 1, so there's no real difference, but semantically, and when considering the approach to take for other tests that may have multi-use mocks, .done is probably a more consistent and reliable assertion.

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py

sarayourfriend · 2024-02-07T02:47:15Z

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py

+            assert data == {
+                "model_name": "sample_model",
+                "table_name": "sample_table",
+                "target_index": "sample_index",
+                "start_id": start_id,
+                "end_id": end_id,
+            }


Rather than pulling the request data out of the pook mock, it's better to tell pook that's what to match in the first place:

pook.post(...).json({ "model_name": "sample_model", "table_name": "sample_table", "target_index": "sample_index", "start_id": start_id, "end_id": end_id, })

If pook sees a request that doesn't match the expected body, it will raise a no match exception, with a detailed diff showing why the actual request didn't match the expected request.

This also lets you avoid needing to think about pook's internals when pulling out the matched request data manually like this, and would remove the need for this complex secondary loop, which could be changed to just [assert mock.done for mock in post_mocks] or, better yet, pook.done, which checks that all mocks registered in context are exhausted.

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

AetherUnbound · 2024-02-07T23:34:52Z

Thanks so much for the idiomatic pook tips! I've applied those, hopefully they look okay :)

openverse-bot · 2024-02-08T00:00:07Z

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@obulat
@stacimc
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@AetherUnbound, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

sarayourfriend

LGMT!

sarayourfriend · 2024-02-08T00:16:34Z

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py

-                "start_id": start_id,
-                "end_id": end_id,
-            }
+        # Pook will raise an exception here if any requests don't match the above


Does pook raise the exception? I think it just returns the boolean 🤔

Sorry, you're right - I'll reword this!

obulat

CI passes and the code looks good.

obulat · 2024-02-08T10:18:49Z

ingestion_server/ingestion_server/distributed_reindex_scheduler.py

+    if failures:
+        raise ValueError(
+            f"Some workers didn't respond to health check: {','.join(failures)}"
+        )


I'm just curious, do you know what value will failures have?

This is the worker URLs that were unable to be reached, see line 58 above!

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py

AetherUnbound added 3 commits January 26, 2024 15:54

Raise an exception if any indexer workers fail their health check

e248565

Add tests for failure case

6df47d3

Add tests for happy path

b1b37e9

AetherUnbound requested a review from a team as a code owner January 31, 2024 01:32

AetherUnbound requested review from obulat and stacimc January 31, 2024 01:32

github-actions bot added the 🧱 stack: ingestion server Related to the ingestion/data refresh server label Jan 31, 2024

openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository labels Jan 31, 2024

AetherUnbound self-assigned this Feb 1, 2024

sarayourfriend approved these changes Feb 7, 2024

View reviewed changes

Simplify pook assertions

3d4a19f

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

sarayourfriend approved these changes Feb 8, 2024

View reviewed changes

obulat approved these changes Feb 8, 2024

View reviewed changes

AetherUnbound commented Feb 8, 2024

View reviewed changes

ingestion_server/test/unit_tests/test_distributed_reindex_scheduler.py Outdated Show resolved Hide resolved

Reword comment to be more accurate

9af0bd3

AetherUnbound merged commit f8e6d35 into main Feb 8, 2024
41 checks passed

AetherUnbound deleted the fix/ingestion-server-raise-exception branch February 8, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise an exception if any indexer workers fail their health check #3726

Raise an exception if any indexer workers fail their health check #3726

AetherUnbound commented Jan 31, 2024

sarayourfriend left a comment

sarayourfriend Feb 7, 2024

sarayourfriend Feb 7, 2024

AetherUnbound commented Feb 7, 2024

openverse-bot commented Feb 8, 2024

sarayourfriend left a comment

sarayourfriend Feb 8, 2024

AetherUnbound Feb 8, 2024

obulat left a comment

obulat Feb 8, 2024

AetherUnbound Feb 8, 2024

Raise an exception if any indexer workers fail their health check #3726

Raise an exception if any indexer workers fail their health check #3726

Conversation

AetherUnbound commented Jan 31, 2024

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 7, 2024

Choose a reason for hiding this comment

sarayourfriend Feb 7, 2024

Choose a reason for hiding this comment

AetherUnbound commented Feb 7, 2024

openverse-bot commented Feb 8, 2024

Footnotes

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 8, 2024

Choose a reason for hiding this comment

AetherUnbound Feb 8, 2024

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

obulat Feb 8, 2024

Choose a reason for hiding this comment

AetherUnbound Feb 8, 2024

Choose a reason for hiding this comment