Skip to content
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

[API] Fix projects leader to sync enrichment to followers #2009

Merged
merged 4 commits into from
Jun 1, 2022

Conversation

Hedingber
Copy link
Contributor

@Hedingber Hedingber commented Jun 1, 2022

Recently we started seeing these tests almost constantly fail in CI:

>               assert (
                    follower._projects[project.metadata.name].status.state
                    == project.spec.desired_state
                )
E               AssertionError: assert None == <ProjectDesiredState.online: 'online'>
E                 +None
E                 -<ProjectDesiredState.online: 'online'>

tests/api/utils/projects/test_leader_member.py:595: AssertionError
FAILED tests/api/utils/projects/test_leader_member.py::test_projects_sync_follower_project_adoption
FAILED tests/api/utils/projects/test_leader_member.py::test_projects_sync_leader_project_syncing
FAILED tests/api/utils/projects/test_leader_member.py::test_projects_sync_multiple_follower_project_adoption

In the investigation we saw that it only reproduces with the dockerized tests and it also reproduces with very old code leading to the conclusions that the cause for this bug is not in our code but some package that was updated (when testing in docker it rebuild the image and pull latest packages vs local which you usually have slightly older versions)
After some searching I found that Pydantic 1.9.0 -> 1.9.1 caused it, and after going over the changes this PR was the suspect - pydantic/pydantic#3642
I've changed the nop follower we're using in those tests to always use .copy(deep=True) of what it is getting and saw that indeed the test now fails even with 1.9.0
This revealed and actual bug in our code, when the leader finds a project in the follower, it enriches it (as of now enrichment is just assigning the desired state to the status state) but then it doesn't update the project in that follower which is why the tests fail, so beside of changing the nop-follower to use deep copy, I fixed the leader to do this update

Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing you found the reason for that annoying CI bug!! minor comment.

):
return
for missing_follower in missing_followers:
if self._should_sync_project_to_followers(project_name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function became really long, and it seems like there are two main scenarios which can be split up to two functions and make the function flow much clearer to follow:

  1. when project doesn't exists in leader
  2. when project doesn't exists in follower

@Tankilevitch Tankilevitch merged commit 93f8bb1 into mlrun:development Jun 1, 2022
Hedingber added a commit that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants