Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions backend/infrahub/core/node/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,16 +212,17 @@ async def extract_peer_data(
obj_peer_data[rel_name] = parent_obj
continue

rel_peer_ids = []
rel_peers = []
for relationship in relationships:
# deeper templates are handled in the next level of recursion
if await _peer_is_a_template(db=db, relationship=relationship):
continue
rel_peer_ids.append({"id": relationship.peer_id})
# The peer is already read, so an id would send the checks and the write back to the database.
rel_peers.append({"id": relationship.get_peer_in_hand() or relationship.peer_id})

# Only set the relationship data if there are actual peers to set
if rel_peer_ids:
obj_peer_data[rel_name] = rel_peer_ids
if rel_peers:
obj_peer_data[rel_name] = rel_peers

if rel_manager.schema.kind == RelationshipKind.PROFILE:
obj_peer_data[rel_name] = peer_ids
Expand Down
24 changes: 5 additions & 19 deletions backend/infrahub/core/query/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@
from infrahub.exceptions import QueryError

if TYPE_CHECKING:
from collections.abc import Mapping

from neo4j.graph import Node as Neo4jNode

from infrahub.core.attribute import AttributeCreateData, BaseAttribute
Expand Down Expand Up @@ -212,25 +210,13 @@ async def query_init(self, db: InfrahubDatabase, **kwargs) -> None: # noqa: ARG
relationships: list[RelationshipCreateData] = []
for rel_name in self.node._relationships:
rel_manager: RelationshipManager = getattr(self.node, rel_name)
peers: Mapping[str, Node] = {}
# This is a create query, so the node has no relationships in the database yet: only
# resolve peers when there are locally-set relationships to write.
if rel_manager.schema.cardinality == "many" and len(rel_manager._relationships):
# Fetch all relationship peers through a single database call for performances.
peers = await rel_manager.get_peers(db=db, branch_agnostic=self.branch_agnostic)
# This is a create query, so the node has no relationships in the database yet: only the
# locally-set relationships are written. Their peers are read in one call, and a peer the
# caller handed over as a node is not read back.
if rel_manager.schema.cardinality == "many":
await rel_manager.read_peers_not_in_hand(db=db, branch_agnostic=self.branch_agnostic)

for rel in rel_manager._relationships:
if rel_manager.schema.cardinality == "many":
try:
rel.set_peer(value=peers[rel.get_peer_id()])
except KeyError:
pass
except ValueError:
# Relationship has not been initialized yet, it means the peer does not exist in db yet
# typically because it will be allocated from a resource pool. In that case, the peer
# will be fetched using `rel.resolve` later.
pass

rel_create_data = await rel.get_create_data(db=db, at=at)
if rel_create_data.peer_branch_level > deepest_branch_level or (
deepest_branch_name == GLOBAL_BRANCH_NAME and rel_create_data.peer_branch == registry.default_branch
Expand Down
40 changes: 33 additions & 7 deletions backend/infrahub/core/relationship/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,17 +192,20 @@ def get_peer_id(self) -> str:
return self.peer_id

def get_peer_kind(self) -> str:
if not self._peer or isinstance(self._peer, str):
return self.schema.peer

return self._peer.get_kind()
peer = self.get_peer_in_hand()
return peer.get_kind() if peer is not None else self.schema.peer

def get_concrete_peer_kind(self) -> str | None:
"""Return the peer's concrete kind, or None when only the schema's (possibly generic) peer kind is known."""
if self._peer and not isinstance(self._peer, str):
return self._peer.get_kind()
peer = self.get_peer_in_hand()
return peer.get_kind() if peer is not None else self._resolved_peer_kind

def get_peer_in_hand(self) -> Node | None:
"""Return the peer as the node it is, or None when this relationship holds only its id."""
if self._peer is None or isinstance(self._peer, str):
return None

return self._resolved_peer_kind
return self._peer

@property
def node_id(self) -> str:
Expand Down Expand Up @@ -1121,6 +1124,29 @@ async def get_peers(
include_metadata=include_metadata,
)

async def read_peers_not_in_hand(self, db: InfrahubDatabase, branch_agnostic: bool = False) -> None:
"""Read, in one query, the peers the relationships hold only the id of, and hand each its node.

A relationship already holding its peer as a node is left as it is: the caller handed the node
over, and reading it back is what this avoids. A relationship without a peer id yet, such as one
waiting on a resource pool, or naming its peer by a default filter value, is left to
`Relationship.resolve()`, which reads it the only way it can be read.
"""
peer_ids = [
rel.peer_id
for rel in self._relationships
if rel.peer_id and is_valid_uuid(rel.peer_id) and rel.get_peer_in_hand() is None
]
if not peer_ids:
return

peers = await registry.manager.get_many(
db=db, ids=peer_ids, branch=self.branch, branch_agnostic=branch_agnostic
)
for rel in self._relationships:
if rel.get_peer_in_hand() is None and rel.peer_id in peers:
rel.set_peer(value=peers[rel.peer_id])

def get_branch_based_on_support_type(self) -> Branch:
"""If the attribute is branch aware, return the Branch object associated with this attribute.

Expand Down
106 changes: 95 additions & 11 deletions backend/tests/component/templates/test_template_reads.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
from __future__ import annotations

from typing import TYPE_CHECKING
from copy import deepcopy
from typing import TYPE_CHECKING, Any

import pytest

from infrahub.core.constants import RelationshipCardinality, RelationshipKind
from infrahub.core.manager import NodeManager
from infrahub.core.node import Node
from infrahub.core.node.create import create_node
from infrahub.core.query.node import NodeListGetInfoQuery, NodeListGetRelationshipsQuery
from infrahub.core.query.node import NodeListGetAttributeQuery, NodeListGetInfoQuery, NodeListGetRelationshipsQuery
from infrahub.core.query.relationship import RelationshipGetPeerQuery
from infrahub.core.registry import registry
from infrahub.core.schema import RelationshipSchema
from tests.constants import TestKind
from tests.helpers.db_query_counter import CountingInfrahubDatabase
from tests.helpers.schema import DEVICE_SCHEMA, load_schema
from tests.helpers.schema import DEVICE_SCHEMA, TAG, load_schema

if TYPE_CHECKING:
from infrahub.core.branch import Branch
Expand All @@ -24,8 +27,37 @@ async def device_schema(db: InfrahubDatabase, default_branch: Branch, register_c
await load_schema(db=db, schema=DEVICE_SCHEMA, branch_name=default_branch.name)


@pytest.fixture(params=[RelationshipCardinality.ONE, RelationshipCardinality.MANY], ids=["one", "many"])
async def device_schema_with_tagged_interfaces(
db: InfrahubDatabase, default_branch: Branch, register_core_models_schema: None, request: pytest.FixtureRequest
) -> None:
"""The device schema, with an interface able to point at a tag: a peer that is not a subtemplate.

Once per cardinality: the create query reads the peers of a relationship of cardinality many in one
batch, a path a single peer never takes.
"""
schema = deepcopy(DEVICE_SCHEMA)
schema.nodes.append(deepcopy(TAG))
physical_interface = next(node for node in schema.nodes if node.kind == TestKind.PHYSICAL_INTERFACE)
physical_interface.relationships.append(
RelationshipSchema(
name="tag",
kind=RelationshipKind.ATTRIBUTE,
optional=True,
peer=TestKind.TAG,
cardinality=request.param,
)
)
await load_schema(db=db, schema=schema, branch_name=default_branch.name)


async def _build_template(
db: InfrahubDatabase, branch: Branch, name: str, nbr_interfaces: int, with_sfp: bool = False
db: InfrahubDatabase,
branch: Branch,
name: str,
nbr_interfaces: int,
with_sfp: bool = False,
tag_id: str | None = None,
) -> Node:
"""Build a device template, optionally giving each of its interfaces a subtemplate of its own."""
template = await Node.init(db=db, schema=f"Template{TestKind.DEVICE}", branch=branch)
Expand All @@ -34,13 +66,15 @@ async def _build_template(

for idx in range(nbr_interfaces):
interface = await Node.init(db=db, schema=f"Template{TestKind.PHYSICAL_INTERFACE}", branch=branch)
await interface.new(
db=db,
template_name=f"{name}-eth{idx}",
name=f"eth{idx}",
phys_type="SFP+ (10GE)",
device=template.id,
)
interface_data: dict[str, Any] = {
"template_name": f"{name}-eth{idx}",
"name": f"eth{idx}",
"phys_type": "SFP+ (10GE)",
"device": template.id,
}
if tag_id:
interface_data["tag"] = tag_id
await interface.new(db=db, **interface_data)
await interface.save(db=db)

if not with_sfp:
Expand Down Expand Up @@ -265,3 +299,53 @@ async def test_a_level_of_a_template_is_read_once_however_many_parents_it_hangs_
# read has no way to be told a node is already in hand. What matters here is that neither figure
# grows with the width of a level.
assert set(node_reads.values()) == {4}, f"reading the subtemplates grew with the width of a level: {node_reads}"


async def test_a_peer_a_component_carries_is_not_read_back_for_every_component(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is a good test 👍 . I put it on the base commit and ran it against the unfixed code. It seems to prove what it tries to.

db: InfrahubDatabase, default_branch: Branch, device_schema_with_tagged_interfaces: None
) -> None:
"""A peer a subtemplate names is handed to the object created from it, not read again per object."""
tag = await Node.init(db=db, schema=TestKind.TAG, branch=default_branch)
await tag.new(db=db, name="uplink")
await tag.save(db=db)

device_schema_obj = registry.schema.get_node_schema(name=TestKind.DEVICE, branch=default_branch)
node_reads = {}
attribute_reads = {}
counts = {}

for nbr_interfaces in (1, 3, 5):
name = f"tagged-{nbr_interfaces}"
template = await _build_template(
db=db, branch=default_branch, name=name, nbr_interfaces=nbr_interfaces, tag_id=tag.id
)
counting_db = CountingInfrahubDatabase.from_db(db=db)

device = await create_node(
data={"name": f"{name}-device", "object_template": {"id": template.id}},
db=counting_db,
branch=default_branch,
schema=device_schema_obj,
)

reloaded = await NodeManager.get_one(db=db, id=device.id, branch=default_branch, raise_on_error=True)
interfaces = await reloaded.interfaces.get_peers(db=db)
assert len(interfaces) == nbr_interfaces
for interface in interfaces.values():
assert [rel.peer_id for rel in await interface.tag.get_relationships(db=db)] == [tag.id], (
"the peer the template names was not written on the objects created from it"
)

node_reads[nbr_interfaces] = counting_db.count_for(NodeListGetInfoQuery.name)
attribute_reads[nbr_interfaces] = counting_db.count_for(NodeListGetAttributeQuery.name)
counts[nbr_interfaces] = sum(counting_db.query_counts.values())

assert counting_db.count_for(RelationshipGetPeerQuery.name) == 0
# The template, the peers its relationships name, and the peers the subtemplate level names:
# three reads, whatever the number of components carrying the tag.
assert set(node_reads.values()) == {3}, f"the tag was read once per component: {node_reads}"
assert set(attribute_reads.values()) == {3}, f"the tag's attributes were read again: {attribute_reads}"
per_component = (counts[3] - counts[1]) / 2
assert per_component == (counts[5] - counts[3]) / 2 == 2, (
f"a component carrying a peer costs {per_component} queries rather than a check and a write: {counts}"
)
1 change: 1 addition & 0 deletions changelog/+template-component-peer-handover.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Creating objects from an object template is now faster when the objects it creates share a related object, such as the transceiver model used by every interface of a device template.
6 changes: 5 additions & 1 deletion dev/knowledge/backend/mutations.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,11 @@ the mutation then works from it rather than reading it back:
(`RelationshipManager.get_peer_id()`) and reads the peer only when it is named by a
human-friendly id or a default filter value, which reading is the only way to resolve;
- the peer-kind constraint trusts the kind a peer states, and reads only the peers named by an id;
- `Relationship.get_create_data()` writes the edge from the peer it holds.
- the create query reads the peers of a relationship of cardinality many in one call, and only those
held by id (`RelationshipManager.read_peers_not_in_hand()`); `Relationship.get_create_data()` then
writes the edge from the peer it holds. Before that, the create query batched every peer of such a
relationship through `get_peers()`, which reads whatever it is given — a component created from a
template read back, once per component, a peer the template read had already brought back.

Passing a node keeps the rest of the payload for that relationship (its source, its owner) only if
the node replaces the `id` inside it rather than the payload itself.
Expand Down
Loading