Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
35 changes: 28 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,24 @@ 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, is left to `Relationship.resolve()`.
"""
peer_ids = [rel.peer_id for rel in self._relationships if rel.peer_id and rel.get_peer_in_hand() is None]
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
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