fix(backend): lock on mutations for branches

We can enable locking for consistency on branches now that they are more
granular and avoid performance penalty.

Signed-off-by: Fatih Acar <[email protected]>

Author: fatih-acar

backend/infrahub/core/node/create.py

@@ -193,7 +193,7 @@ async def create_node(
     preview_obj = await node_class.init(db=db, schema=schema, branch=branch)
     await preview_obj.new(db=db, process_pools=False, **data)
     schema_branch = db.schema.get_schema_branch(name=branch.name)
-    lock_names = get_lock_names_on_object_mutation(node=preview_obj, branch=branch, schema_branch=schema_branch)
+    lock_names = get_lock_names_on_object_mutation(node=preview_obj, schema_branch=schema_branch)
 
     async def _persist(current_db: InfrahubDatabase) -> Node:
         node_constraint_runner = await component_registry.get_component(

backend/infrahub/core/node/save.py

@@ -41,7 +41,7 @@ async def run_constraints_and_save(
     locks = (
         list(lock_names)
         if lock_names is not None
-        else get_lock_names_on_object_mutation(node=node, branch=branch, schema_branch=schema_branch)
+        else get_lock_names_on_object_mutation(node=node, schema_branch=schema_branch)
     )
 
     async def _persist() -> None:

backend/infrahub/graphql/mutations/ipam.py

@@ -107,10 +107,7 @@ def __init_subclass_with_meta__(
         super().__init_subclass_with_meta__(_meta=_meta, **options)
 
     @staticmethod
-    def _get_lock_names(namespace_id: str, branch: Branch) -> list[str]:
-        if not branch.is_default:
-            # Do not lock on other branches as reconciliation will be performed at least when merging in main branch.
-            return []
+    def _get_lock_names(namespace_id: str) -> list[str]:
         return [build_object_lock_name(InfrahubKind.IPADDRESS + "_" + namespace_id)]
 
     @classmethod
@@ -144,7 +141,7 @@ async def mutate_create(
         ip_address = ipaddress.ip_interface(data["address"]["value"])
         namespace_id = await validate_namespace(db=db, branch=branch, data=data)
 
-        lock_names = cls._get_lock_names(namespace_id, branch)
+        lock_names = cls._get_lock_names(namespace_id)
         async with InfrahubMultiLock(lock_registry=lock.registry, locks=lock_names):
             async with db.start_transaction() as dbt:
                 reconciled_address = await cls._mutate_create_object_and_reconcile(
@@ -222,9 +219,9 @@ async def mutate_update(
                 fields.remove(field_to_remove)
 
         schema_branch = db.schema.get_schema_branch(name=branch.name)
-        lock_names = get_lock_names_on_object_mutation(node=address, branch=branch, schema_branch=schema_branch)
+        lock_names = get_lock_names_on_object_mutation(node=address, schema_branch=schema_branch)
 
-        namespace_lock_names = cls._get_lock_names(namespace_id, branch)
+        namespace_lock_names = cls._get_lock_names(namespace_id)
         async with InfrahubMultiLock(lock_registry=lock.registry, locks=namespace_lock_names):
             async with InfrahubMultiLock(lock_registry=lock.registry, locks=lock_names):
                 async with db.start_transaction() as dbt:
@@ -399,7 +396,7 @@ async def mutate_update(
                 fields.remove(field_to_remove)
 
         schema_branch = db.schema.get_schema_branch(name=branch.name)
-        lock_names = get_lock_names_on_object_mutation(node=prefix, branch=branch, schema_branch=schema_branch)
+        lock_names = get_lock_names_on_object_mutation(node=prefix, schema_branch=schema_branch)
 
         namespace_lock_names = cls._get_lock_names(namespace_id)
         async with InfrahubMultiLock(lock_registry=lock.registry, locks=namespace_lock_names):

backend/infrahub/graphql/mutations/main.py

@@ -230,7 +230,7 @@ async def _call_mutate_update(
         await preview_obj.from_graphql(db=db, data=data, process_pools=False)
 
         schema_branch = db.schema.get_schema_branch(name=branch.name)
-        lock_names = get_lock_names_on_object_mutation(node=preview_obj, branch=branch, schema_branch=schema_branch)
+        lock_names = get_lock_names_on_object_mutation(node=preview_obj, schema_branch=schema_branch)
 
         async def _mutate(current_db: InfrahubDatabase) -> tuple[Node, Self]:
             updated_obj = await cls.mutate_update_object(
@@ -332,7 +332,7 @@ async def mutate_update_object(
         locks = lock_names
         if locks is None:
             schema_branch = db.schema.get_schema_branch(name=branch.name)
-            locks = get_lock_names_on_object_mutation(node=obj, branch=branch, schema_branch=schema_branch)
+            locks = get_lock_names_on_object_mutation(node=obj, schema_branch=schema_branch)
 
         await run_constraints_and_save(
             node=obj,

backend/infrahub/lock_getter.py

@@ -1,8 +1,6 @@
 import hashlib
 from typing import TYPE_CHECKING
 
-from infrahub.core.branch import Branch
-from infrahub.core.constants import InfrahubKind
 from infrahub.core.node import Node
 from infrahub.core.schema import GenericSchema
 from infrahub.core.schema.schema_branch import SchemaBranch
@@ -11,8 +9,6 @@
 if TYPE_CHECKING:
     from infrahub.core.relationship import RelationshipManager
 
-KINDS_CONCURRENT_MUTATIONS_NOT_ALLOWED = [InfrahubKind.GENERICGROUP]
-
 
 def _get_kinds_to_lock_on_object_mutation(kind: str, schema_branch: SchemaBranch) -> list[str]:
     """
@@ -48,31 +44,13 @@ def _get_kinds_to_lock_on_object_mutation(kind: str, schema_branch: SchemaBranch
     return kinds
 
 
-def _should_kind_be_locked_on_any_branch(kind: str, schema_branch: SchemaBranch) -> bool:
-    """
-    Check whether kind or any kind generic is in KINDS_TO_LOCK_ON_ANY_BRANCH.
-    """
-
-    if kind in KINDS_CONCURRENT_MUTATIONS_NOT_ALLOWED:
-        return True
-
-    node_schema = schema_branch.get(name=kind)
-    if isinstance(node_schema, GenericSchema):
-        return False
-
-    for generic_kind in node_schema.inherit_from:
-        if generic_kind in KINDS_CONCURRENT_MUTATIONS_NOT_ALLOWED:
-            return True
-    return False
-
-
 def _hash(value: str) -> str:
     # Do not use builtin `hash` for lock names as due to randomization results would differ between
     # different processes.
     return hashlib.sha256(value.encode()).hexdigest()
 
 
-def get_lock_names_on_object_mutation(node: Node, branch: Branch, schema_branch: SchemaBranch) -> list[str]:
+def get_lock_names_on_object_mutation(node: Node, schema_branch: SchemaBranch) -> list[str]:
     """
     Return lock names for object on which we want to avoid concurrent mutation (create/update). Except for some specific kinds,
     concurrent mutations are only allowed on non-main branch as objects validations will be performed at least when merging in main branch.
@@ -99,9 +77,6 @@ def _add_pool_lock(pool_id: str) -> None:
             if rel.from_pool and "id" in rel.from_pool:
                 _add_pool_lock(rel.from_pool["id"])
 
-    if not branch.is_default and not _should_kind_be_locked_on_any_branch(node.get_kind(), schema_branch):
-        return lock_names
-
     lock_kinds = _get_kinds_to_lock_on_object_mutation(node.get_kind(), schema_branch)
     for kind in lock_kinds:
         schema = schema_branch.get(name=kind)

backend/tests/unit/core/test_get_kinds_lock.py

@@ -6,7 +6,6 @@
 from infrahub.lock_getter import (
     _get_kinds_to_lock_on_object_mutation,
     _hash,
-    _should_kind_be_locked_on_any_branch,
     get_lock_names_on_object_mutation,
 )
 from tests.helpers.test_app import TestInfrahubApp
@@ -48,19 +47,7 @@ async def test_lock_other_branch(
         schema_branch = registry.schema.get_schema_branch(name=other_branch.name)
 
         person = await create_and_save(db=db, schema="TestPerson", name="John", branch=other_branch)
-        assert get_lock_names_on_object_mutation(person, branch=other_branch, schema_branch=schema_branch) == []
-
-    async def test_lock_groups_on_other_branches(
-        self,
-        db: InfrahubDatabase,
-        default_branch,
-        client,
-        register_core_models_schema,
-    ):
-        other_branch = await create_branch(branch_name="other_branch", db=db)
-        schema_branch = registry.schema.get_schema_branch(name=other_branch.name)
-
-        assert _should_kind_be_locked_on_any_branch(kind="CoreGraphQLQueryGroup", schema_branch=schema_branch) is True
+        assert get_lock_names_on_object_mutation(person, schema_branch=schema_branch) == ["global.object.TestPerson." + _hash("John")]
 
     async def test_lock_names_only_attributes(
         self,
@@ -78,7 +65,7 @@ async def test_lock_names_only_attributes(
         schema_branch = registry.schema.get_schema_branch(name=default_branch.name)
         person = await create_and_save(db=db, schema="TestPerson", name="John")
         car = await create_and_save(db=db, schema="TestCar", name="mercedes", color="blue", owner=person)
-        assert get_lock_names_on_object_mutation(car, branch=default_branch, schema_branch=schema_branch) == [
+        assert get_lock_names_on_object_mutation(car, schema_branch=schema_branch) == [
             "global.object.TestCar." + _hash("mercedes") + "." + _hash("blue")
         ]
 
@@ -95,6 +82,6 @@ async def test_lock_names_optional_empty_attribute(
 
         schema_branch = registry.schema.get_schema_branch(name=default_branch.name)
         person = await create_and_save(db=db, schema="TestPerson", name="John")
-        assert get_lock_names_on_object_mutation(person, branch=default_branch, schema_branch=schema_branch) == [
+        assert get_lock_names_on_object_mutation(person, schema_branch=schema_branch) == [
             "global.object.TestPerson." + _hash("") + "." + _hash("John")
         ]