-
Notifications
You must be signed in to change notification settings - Fork 57
fix(backend): branch registry updates #10396
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
base: stable
Are you sure you want to change the base?
Changes from 4 commits
35c5373
2952c90
7fcbbf6
a52649c
6a3dea9
a79eeaf
98e877f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from infrahub.core.constants import GLOBAL_BRANCH_NAME | ||
| from infrahub.graphql.registry import registry as graphql_registry | ||
| from infrahub.log import get_logger | ||
| from infrahub.utils import log_exception_guard | ||
| from infrahub.worker import WORKER_IDENTITY | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -95,10 +96,12 @@ async def refresh_branches(db: InfrahubDatabase) -> None: | |
| # have an associated schema | ||
| continue | ||
|
|
||
| if active_branch.name in registry.branch: | ||
| await update_branch_registry(db=db, branch=active_branch) | ||
| else: | ||
| await create_branch_registry(db=db, branch=active_branch) | ||
| # Absorb a failure on one branch rather than abandoning the sweep | ||
| with log_exception_guard(log, f"Failed to refresh branch {active_branch.name!r} in the registry"): | ||
| if active_branch.name in registry.branch: | ||
| await update_branch_registry(db=db, branch=active_branch) | ||
| else: | ||
| await create_branch_registry(db=db, branch=active_branch) | ||
|
Comment on lines
+104
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow the fact that these two functions have the same number of characters and that only the first words differ confused me. I had to read it 5 times to finally see they are 2 different functions 🫨 |
||
|
|
||
| purged_branches = await registry.purge_inactive_branches(db=db, active_branches=active_branches) | ||
| purged_branches.update( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| from uuid import uuid4 | ||
|
|
||
| from fast_depends import Provider | ||
|
|
||
| from infrahub.auth.session import AccountSession | ||
| from infrahub.auth.types import AuthType | ||
| from infrahub.context import InfrahubContext | ||
| from infrahub.core import registry | ||
| from infrahub.core.branch import Branch | ||
| from infrahub.core.branch.enums import BranchStatus | ||
| from infrahub.core.branch.tasks import migrate_branch | ||
| from infrahub.core.graph import GRAPH_VERSION | ||
| from infrahub.core.initialization import create_branch | ||
| from infrahub.core.schema.schema_branch import SchemaBranch | ||
| from infrahub.database import InfrahubDatabase | ||
| from infrahub.workers.dependencies import build_database | ||
|
|
||
|
|
||
| async def test_migrate_branch_publishes_migrated_branch( | ||
| db: InfrahubDatabase, | ||
| default_branch: Branch, | ||
| car_person_schema: SchemaBranch, | ||
| dependency_provider: Provider, | ||
| ) -> None: | ||
| """The flow migrates an instance of its own, so it has to publish that instance itself.""" | ||
| branch = await create_branch(db=db, branch_name="migrate-branch") | ||
| assert registry.branch[branch.name] is branch | ||
|
|
||
| # A branch as an upgrade leaves it behind: its graph version trails the application | ||
| branch.graph_version = GRAPH_VERSION - 1 | ||
| await branch.save(db=db) | ||
|
|
||
| context = InfrahubContext.init( | ||
| branch=default_branch, | ||
| account=AccountSession(account_id=str(uuid4()), auth_type=AuthType.NONE), | ||
| ) | ||
| with dependency_provider.scope(build_database, lambda singleton=True: db): # noqa: ARG005 | ||
| await migrate_branch(branch=branch.name, context=context, send_events=False) | ||
|
|
||
| migrated_branch = await Branch.get_by_name(db=db, name=branch.name) | ||
| assert migrated_branch.graph_version == GRAPH_VERSION | ||
| assert migrated_branch.status is BranchStatus.OPEN | ||
|
|
||
| published_branch = registry.branch[branch.name] | ||
| assert published_branch.graph_version == GRAPH_VERSION | ||
| assert published_branch.status is BranchStatus.OPEN |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import pytest | ||
|
|
||
| from infrahub.core import registry | ||
| from infrahub.core.branch import Branch | ||
| from infrahub.core.constants import NULL_VALUE | ||
| from infrahub.core.initialization import create_branch | ||
| from infrahub.core.schema.schema_branch import SchemaBranch | ||
| from infrahub.core.timestamp import Timestamp | ||
| from infrahub.database import InfrahubDatabase | ||
| from infrahub.tasks.registry import refresh_branches | ||
|
|
||
|
|
||
| async def test_refresh_branches_continues_past_a_branch_it_cannot_refresh( | ||
| db: InfrahubDatabase, | ||
| default_branch: Branch, | ||
| car_person_schema: SchemaBranch, | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """The sweep is the only thing that repairs a stale cache entry, so one bad branch must not end it.""" | ||
| broken_branch = await create_branch(db=db, branch_name="broken-branch") | ||
| stale_branch = await create_branch(db=db, branch_name="stale-branch") | ||
|
|
||
| # A branch row whose schema hash never reached storage — how the global branch and a graph | ||
| # predating the field look. Reading it back raises from Branch.active_schema_hash | ||
| await db.execute_query( | ||
| query="MATCH (n:Branch {name: $branch_name}) SET n.schema_hash = $null_value", | ||
| params={"branch_name": broken_branch.name, "null_value": NULL_VALUE}, | ||
| ) | ||
| assert (await Branch.get_by_name(db=db, name=broken_branch.name)).schema_hash is None | ||
|
|
||
| # Something for the sweep to pick up: a rebase timestamp that only exists in the database | ||
| rebased_branch = await Branch.get_by_name(db=db, name=stale_branch.name) | ||
| rebased_branch.branched_from = Timestamp().to_string() | ||
| await rebased_branch.save(db=db) | ||
| assert registry.branch[stale_branch.name].branched_from != rebased_branch.branched_from | ||
|
|
||
| with caplog.at_level("ERROR", logger="infrahub"): | ||
| await refresh_branches(db=db) | ||
|
|
||
| # The branch it gave up on has to be reported, with its traceback: absorbed is not silent | ||
| failures = [ | ||
| record.msg | ||
| for record in caplog.records | ||
| if isinstance(record.msg, dict) | ||
| and record.msg.get("event") == f"Failed to refresh branch '{broken_branch.name}' in the registry" | ||
| ] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this reuse
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes good idea. this is done |
||
| assert len(failures) == 1 | ||
| assert failures[0]["level"] == "error" | ||
| assert failures[0]["exc_info"] | ||
|
|
||
| assert registry.branch[stale_branch.name].branched_from == rebased_branch.branched_from | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Update the in-memory cache for branches after a branch change is saved to prevent the cache diverging from the database. The BranchUpdate mutation and the Prefect task to run database migrations against a branch are both fixed. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| A branch that cannot be refreshed into a worker's in-memory registry no longer aborts the periodic branch refresh. The failure is logged and the remaining branches are still refreshed. |
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.
Is this safe the branch is not already in this worker's registry?
Branch.get_by_namenever loads the schema, andregistry.get_branch()andcreate_branch_registry()both do when they insert.If the entry lands schema-less, does the next
refresh_branchesever fix it?Is this where the
BranchSaveryou mention in the description would come in handy?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.
very good point. updated all the registry update sites to only update an existing branch in the registry and not add a new one. I think if everything is working correctly, then this would not be an issue that we could encounter b/c the branches and their schemas will always be up-to-date, but it is better to be safe