Fix IPPrefix utilisation on new branch#9230
Conversation
A freshly created branch reported zero utilization for prefixes that had non-zero utilization on the origin branch. The resolver asked for utilization only on the current branch name, so data inherited from the origin branch was filtered out. It now queries across the full inheritance scope so inherited allocations remain counted until the branch modifies them.
github-actions
Bot
added
the
group/backend
gmazoyer
force-pushed
the
gma-20260512-9220
branch
from
9b95ae0 to
87c5d69
Compare
This should be better suited to fetch the utilisation of a prefix without mixing up what's in the default branch and the other one.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 6 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would require human review. This PR modifies the core IPAM utilization query logic to consider branch inheritance, which is a business logic change in a critical path that could affect data integrity across branches, so human review is warranted.
Re-trigger cubic
|
|
||
| def rel_filter(rel_name: str) -> str: | ||
| return f"{rel_name}.from <= $time_at AND ({rel_name}.to IS NULL OR {rel_name}.to >= $time_at)" | ||
| branch = self.branch |
There was a problem hiding this comment.
I spent longer than I should have trying to clean this query up to be more in line with how we do cypher now (replace the %(id_func)s with elementId, use string-interpolation instead of an f-string) and to support getting both default branch and user branch utilization at once, but I think it is probably better to just rewrite this thing to work for a single branch and then call it twice.
I can look into that later this week or next week if you don't have the time for it now
There was a problem hiding this comment.
Tried my luck a rewriting the query with your comment in mind. It can now only be used with a single branch.
The prefix utilisation query previously served two purposes from one dual-purpose Cypher: a per-branch visibility view and a cross-branch attribution view selected by a `branch_names` filter on the caller. That entangled the two semantics, required custom branch and time handling that diverged from how the rest of the codebase queries the graph, and made the deepest-path deduplication fragile, a branch-side deletion could leak into the origin's view. The query is now scoped to a single branch using the standard branch filter helper, so isolation, time clamping at `branched_from` and the existing path filter pattern all apply uniformly. Callers that need a cross-branch picture, currently only the pool view, instantiate one getter per branch and combine the results themselves.
gmazoyer
force-pushed
the
gma-20260512-9220
branch
from
38a4913 to
8f0e63c
Compare
gmazoyer
changed the title
A branch can now see less than the default branch on the same prefix, so clamp the subtraction-derived field at zero.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR refactors the core IPAM utilization query to use standard branch scoping, which changes how utilization is calculated across branches; such logic changes in critical IPAM functionality require human review to ensure correctness and to catch any edge cases missed by automated checks.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR refactors the core IPAM utilization query and data flow to fix branch isolation bugs, which involves non-trivial changes to business logic and database queries, so it requires human review to ensure correctness across all branch scenarios.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. This pull request modifies core IPAM utilization logic with significant refactoring of database queries and branch scoping in six files, which is high-risk and requires human review to verify correctness and prevent regressions in production.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. This PR modifies core IPAM utilization logic and database queries to fix cross-branch counting bugs, which is a high-impact change affecting data integrity and production infrastructure, so a human reviewer must verify correctness.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. The PR modifies core IP prefix utilization logic across branch scoping, affecting both queries and GraphQL resolvers, and given the complexity and potential impact on data integrity, human review is necessary.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. This change rewrites the core IPAM utilization branch-scoping logic across multiple files (query, getter, graphql resolvers), touching 543 lines with non-trivial refactoring that could affect production data integrity if incorrect.
Re-trigger cubic
gmazoyer
force-pushed
the
gma-20260512-9220
branch
from
18b7d41 to
da3609b
Compare
|
Summary of the changes to the Cypher:
Both The query never counts the parent prefix itself as one of its own children. The The deepest-active grouping is keyed on Some polish:
|
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
the suggested cypher changes are optional. this is a big improvement as is
| WITH pfx, pfx_root | ||
| WHERE pfx_root.status = "active" | ||
| WITH pfx | ||
| CALL (pfx) { |
There was a problem hiding this comment.
I think that this subquery works correctly, but it would probably perform better to break it up into 2 or 4 subqueries that filtered out deleted/inactive edges along the way. so it would be something like
MATCH (pfx)-[:IS_RELATED]-(rl:Relationship)
WHERE rl.name IN $allocated_kinds_rel
WITH DISTINCT pfx, rl
CALL (pfx, rl) {
MATCH (pfx)-[r:IS_RELATED]-(rl)
WHERE %(branch_filter)
RETURN r AS r_rel1
ORDER BY r.branch_level DESC, r.from DESC, r.status ASC
LIMIT 1
}
WITH pfx, r_rel1, rl
WHERE r_rel1.status = "active"
CALL (rl) {
MATCH (rl:Relationship)<-[r:IS_RELATED]-(child:%(allocated_labels)s)
WHERE %(branch_filter)
RETURN r AS r_rel2, child
ORDER BY r.branch_level DESC, r.from DESC, r.status ASC
LIMIT 1
}
WITH pfx, r_rel1, rl, r_rel2, child
WHERE r_rel2.status = "active"
CALL (child) {
MATCH (child)-[r:HAS_ATTRIBUTE]->(attr:Attribute)
WHERE attr.name IN ["prefix", "address"]
AND %(branch_filter)
RETURN r AS r_attr, attr
ORDER BY r.branch_level DESC, r.from DESC, r.status ASC
LIMIT 1
}
WITH pfx, r_rel1, rl, r_rel2, child, r_attr, attr
WHERE r_attr.status = "active"
// ... and then one more for the HAS_VALUE edge
// and you might be able to drop the r_rel1, r_rel2, etc. along the way
// then you know that edge row in the result set is a single "child" and its active prefix/address value
| WHERE is_latest_active | ||
| RETURN child, av | ||
| } | ||
| CALL (child) { |
There was a problem hiding this comment.
I don't think you need this part. if there's an active relationship to the Node we can assume that the Node itself is active
Picking the deepest-active edge per hop keeps intermediate cardinality bounded and lets the active IS_RELATED edge stand in for the child's own aliveness check.
gmazoyer
force-pushed
the
gma-20260512-9220
branch
2 times, most recently
from
9becc75 to
8b0b3e4
Compare
2 participants
Why & What
IpamIPPrefix.utilizationreturned wrong values on every non-default branch:0for every prefix that had non-zero utilisation on its origin (the issue this PR was opened for).The cause was that prefix utilisation had its own bespoke way of reading the graph that did not match how the rest of the platform handles branches. Both per-branch visibility and cross-branch attribution were squeezed into a single query, which mixed the two semantics and routinely produced wrong results when modifications existed on both sides.
Utilisation now reads from one branch at a time using the same branch scoping that powers every other read in Infrahub: an isolated branch sees its origin frozen at its creation point and adds its own changes on top. The pool view, which still needs both the current branch view and the default branch view, derives them by asking the same question twice and subtracting.
Closes #9220
Checklist
uv run towncrier create ...)