Keep a fabric's node IDs on the cluster, and check the whole chain there

- The node-ID pool lives in a ConfigMap the Fabric composes;
  spec.nodeIdPool.seedConfigMapName seeds it from a fabric running elsewhere.
- function/avd_compat.py: AVD's v2.x spine addressing as an AvdIpAddressing
  subclass, where pyavd implements no Jinja templating.
- avd-migrate reports what it cannot carry: pool assignments, play-level vars.
  Fabric names are unique per play now.
- Fix: the Fabric XRD required spec.design, which avd-migrate never emits.
- Fix: uppercase hostnames could not be composed.
- Fix: scripts/kind-up.sh installed one XRD out of six.
- e2e: all eight bundled AVD examples migrate, apply and render AVD's golden on
  a cluster -- every device, one namespace each.
- tests/test_e2e_node_id_pool.py is strict-xfail: twodc is 26/26 offline and 12
  differences on a cluster. Not diagnosed.
- 91 offline tests green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: mbakalarski

apis/fabric/xrd.yaml

@@ -88,6 +88,35 @@ spec:
                     required:
                       - kind
                       - name
+                nodeIdPool:
+                  type: object
+                  description: >-
+                    Where this fabric's node IDs come from when the design sets
+                    fabric_numbering.node_id.algorithm to pool_manager. AVD then
+                    hands out IDs instead of reading them off each node, and
+                    keeps the assignments in a file. There is no such file in a
+                    cluster, so the Fabric composes a ConfigMap and reads it back
+                    on the next reconcile. Nothing here is needed for a fabric
+                    that numbers its nodes itself.
+                  properties:
+                    seedConfigMapName:
+                      type: string
+                      description: >-
+                        A ConfigMap holding assignments to start from, read once
+                        when this fabric has no pool of its own yet. It exists
+                        for a fabric that was already running elsewhere: AVD
+                        generated its IDs and they are in a file, and a fabric
+                        that starts over assigns different ones - which reaches
+                        every device as a full configuration replacement. Not
+                        needed for a new fabric, and ignored once the composed
+                        pool exists, so it seeds rather than overrides.
+                      minLength: 1
+                    seedKey:
+                      type: string
+                      description: >-
+                        Key within that ConfigMap. Defaults to the key the
+                        composed pool uses.
+                      minLength: 1
                 push:
                   type: object
                   description: >-
@@ -126,9 +155,14 @@ spec:
                       description: cEOS serves a self-signed cert; default true.
                   required:
                     - credentialsSecretName
+              # `design` is NOT required: a fabric may be assembled entirely from
+              # the inputs named in `spec.requires`, which is what `avd-migrate`
+              # emits and what `fn.py` has always accepted ("design or requires").
+              # Requiring it here made every migrated Fabric unapplyable, and no
+              # offline test could see it -- they drive RunFunction directly and
+              # never meet the API server's schema.
               required:
                 - fabricName
-                - design
             status:
               type: object
               properties:

function/avd_compat.py

@@ -0,0 +1,59 @@
+"""AVD behaviours pyavd cannot reach, written as the classes AVD asks for.
+
+pyavd implements **no Jinja templating**: `get_device_structured_config` passes
+`templar=None` and the call raises `NotImplementedError`, and no public entry
+point accepts a templar. So a design pinning a `.j2` path cannot render here,
+wherever the file is carried.
+
+The same schema blocks — `node_type_keys[].ip_addressing` and
+`.interface_descriptions` — take `python_module` / `python_class_name` instead,
+and **that route pyavd supports**: `load_python_class` imports the module by
+dotted path and checks it against the public base class. A module shipped inside
+this package is importable by dotted path, so pointing a design at
+`function.avd_compat` loads no arbitrary code — it loads ours.
+
+This is not a general answer. It reproduces one specific, published scheme.
+Anyone whose fabric uses a template of their own writes their own class and
+builds their own function image on this one.
+"""
+
+from __future__ import annotations
+
+import ipaddress
+
+from pyavd.api.ip_addressing import AvdIpAddressing
+
+
+class AvdIpAddressingV2Spine(AvdIpAddressing):
+    """AVD v2.x spine-to-super-spine P2P addressing.
+
+    A transcription of the two templates `eos_designs-twodc-5stage-clos` pins,
+    which say what they are: *"In AVD v2.x the spine to super-spine links used
+    this special IP addressing scheme. This file may still be used by older
+    inventories."*
+
+    ⚠ The comment describes where they came from, not that they are inert. The
+    scheme divides the uplink pool by `max_uplink_switches` so that adding a
+    spine does not move existing addresses — which is why that fabric's golden
+    puts a spine's two super-spine uplinks 64 apart rather than adjacent. AVD's
+    native algorithm packs them contiguously and the two are not interchangeable.
+
+    Everything else falls through to `AvdIpAddressing`, so a fabric selecting
+    this class changes only its P2P uplinks.
+    """
+
+    def _v2_p2p(self, uplink_switch_index: int, last: int) -> str:
+        pool = ipaddress.ip_network(self._uplink_ipv4_pool, strict=False)
+        offset = (self._id - 1) % self._max_parallel_uplinks
+        index = (
+            (pool.num_addresses // self._max_uplink_switches) * int(uplink_switch_index)
+            + ((self._id - 1) * self._max_parallel_uplinks + offset) * 2
+            + last
+        )
+        return str(pool.network_address + index)
+
+    def p2p_uplinks_ip(self, uplink_switch_index: int) -> str:
+        return self._v2_p2p(uplink_switch_index, 1)
+
+    def p2p_uplinks_peer_ip(self, uplink_switch_index: int) -> str:
+        return self._v2_p2p(uplink_switch_index, 0)

function/engine.py

@@ -85,19 +85,27 @@ def validate_all(all_inputs: dict[str, dict]) -> dict[str, list]:
 
 
 def render_structured_configs(
-    all_inputs: dict[str, dict], *, validate: bool = True
+    all_inputs: dict[str, dict], *, validate: bool = True, pool_manager: object = None
 ) -> dict[str, dict]:
     """Run facts + per-device structured config for the whole fabric.
 
     ``get_avd_facts`` is fabric-wide (needs every device at once); the structured
     config is then derived per device from those shared facts.
+
+    ``pool_manager`` is required only by a fabric setting
+    ``fabric_numbering.node_id.algorithm: pool_manager``, which asks AVD to
+    assign node IDs from a pool instead of reading them off each node. ⚠ The
+    pool is **a file** (`pyavd.api.pool_manager.PoolManager(output_dir)`), and
+    the assignments have to survive between runs or every device is renumbered —
+    so nothing composes one yet, and a Fabric that asks for it fails with AVD's
+    own message until a Fabric has somewhere to keep it.
     """
     if validate:
         violations = validate_all(all_inputs)
         if violations:
             raise InputValidationError(violations)
 
-    avd_facts = pyavd.get_avd_facts(all_inputs)
+    avd_facts = pyavd.get_avd_facts(all_inputs, pool_manager=pool_manager)
     return {
         hostname: pyavd.get_device_structured_config(
             hostname, inputs, avd_facts=avd_facts

function/fn.py

@@ -17,6 +17,7 @@
 from __future__ import annotations
 
 import hashlib
+import re
 from datetime import datetime, timezone
 
 import pyavd
@@ -25,7 +26,7 @@
 from crossplane.function.proto.v1 import run_function_pb2 as fnv1
 from crossplane.function.proto.v1 import run_function_pb2_grpc as grpcv1
 
-from . import push
+from . import pools, push
 from .engine import (
     InputValidationError,
     device_roles_from_design,
@@ -64,6 +65,19 @@ def _normalize_numbers(obj):
     return obj
 
 
+def _dns_name(hostname: str) -> str:
+    """A hostname as a Kubernetes object name may spell it.
+
+    AVD hostnames are free text and two of its eight bundled examples --
+    `campus-fabric` and `l2ls-fabric` -- write them entirely in capitals. A
+    composed resource named after one is rejected outright: *"invalid name
+    ... Must be a valid RFC 1123 subdomain name"*, so the whole fabric fails to
+    compose. The hostname itself is untouched; only the object's name is spelled
+    this way.
+    """
+    return re.sub(r"[^a-z0-9-]+", "-", hostname.lower()).strip("-") or "device"
+
+
 def _now() -> str:
     return datetime.now(timezone.utc).isoformat(timespec="seconds")
 
@@ -166,8 +180,47 @@ def _reconcile_fabric(
             document["fabric_name"] = fabric_name
             all_inputs = {host: document for host in hostnames_from_design(document)}
 
+        # A fabric that asks for pool-assigned node IDs needs its assignments
+        # back before it renders, or every device is renumbered on every pass.
+        keeps_a_pool = pools.wanted_by(all_inputs)
+        pool = ""
         try:
-            structured_configs = render_structured_configs(all_inputs)
+            if keeps_a_pool:
+                previous = pools.observed_pool(req.observed.resources)
+                if not previous.strip():
+                    # Only before this fabric has a pool of its own. Once it has,
+                    # the seed is history and must not override it.
+                    state, seeded = pools.seed(req, rsp, spec, namespace)
+                    if state == "pending":
+                        # The normal first pass, exactly as for the named inputs.
+                        response.set_conditions(
+                            rsp,
+                            resource.Condition(
+                                typ="InputsResolved",
+                                status="False",
+                                reason="WaitingForSeed",
+                                message="waiting for the node-ID pool seed ConfigMap",
+                            ),
+                        )
+                        response.normal(rsp, "waiting for the node-ID pool seed")
+                        return
+                    if state == "missing":
+                        response.fatal(
+                            rsp,
+                            "spec.nodeIdPool.seedConfigMapName names a ConfigMap that "
+                            "does not exist; rendering without it would renumber every "
+                            "device",
+                        )
+                        return
+                    previous = seeded
+                with pools.pool_manager(all_inputs, previous) as (manager, pool_file):
+                    structured_configs = render_structured_configs(
+                        all_inputs, pool_manager=manager
+                    )
+                    manager.save_updated_pools()
+                    pool = pool_file.read_text() if pool_file.is_file() else previous
+            else:
+                structured_configs = render_structured_configs(all_inputs)
         except InputValidationError as err:
             resource.update_status(
                 rsp.desired.composite,
@@ -179,6 +232,15 @@ def _reconcile_fabric(
             response.fatal(rsp, f"AVD render failed: {type(err).__name__}: {err}")
             return
 
+        if keeps_a_pool:
+            # Composed after the render, so a failed render never overwrites a
+            # good pool with a partial one.
+            resource.update(
+                rsp.desired.resources[pools.RESOURCE_NAME],
+                pools.configmap(xr_name, namespace, fabric_name, pool),
+            )
+            rsp.desired.resources[pools.RESOURCE_NAME].ready = fnv1.READY_TRUE
+
         push_spec = spec.get("push") or {}
         if push_spec and not push_spec.get("credentialsSecretName"):
             response.fatal(rsp, "spec.push.credentialsSecretName is required when push is set")
@@ -197,6 +259,20 @@ def _reconcile_fabric(
             host: device_roles_from_design(hostvars).get(host) or hostvars.get("type", "")
             for host, hostvars in all_inputs.items()
         }
+        # Two hostnames that differ only in case, or only in a character a
+        # Kubernetes name cannot carry, would compose a single Device between
+        # them -- one config silently standing in for two switches.
+        spelled: dict[str, str] = {}
+        for hostname in sorted(structured_configs):
+            clash = spelled.setdefault(_dns_name(hostname), hostname)
+            if clash != hostname:
+                response.fatal(
+                    rsp,
+                    f"devices {clash!r} and {hostname!r} both need the object name "
+                    f"{_dns_name(hostname)!r}; rename one",
+                )
+                return
+
         observed_devices = req.observed.resources  # keyed by composition-resource-name (hostname)
         devices = []
         for hostname, structured_config in structured_configs.items():
@@ -207,7 +283,7 @@ def _reconcile_fabric(
                 "apiVersion": API_VERSION,
                 "kind": "Device",
                 "metadata": {
-                    "name": resource.child_name(xr_name, hostname),
+                    "name": resource.child_name(xr_name, _dns_name(hostname)),
                     "namespace": namespace,
                     "labels": {
                         "avd.netclab.dev/fabric": fabric_name,

function/migrate.py

@@ -294,10 +294,20 @@ def _inputs(fragments: list[Fragment], devices: frozenset[str],
     return [inp for _, inp in planned]
 
 
-def _fabric_name(root: Path, play: Play, many: bool) -> str:
+def _fabric_name(root: Path, play: Play, many: bool, taken: set[str]) -> str:
+    """A name per play, and never the same one twice.
+
+    ⚠ The pattern does not always tell two plays apart:
+    `eos_designs-twodc-5stage-clos` runs eos_designs twice over the same
+    `hosts: TWODC_5STAGE_CLOS`, so naming by pattern gave both fabrics one name —
+    and `--emit` wrote one file, the second silently overwriting the first.
+    """
     if not many:
-        return slug(root.name)
-    return slug(f"{root.name}-{play.pattern}") or slug(f"{root.name}-{play.index}")
+        return _unique(slug(root.name), taken)
+    base = slug(f"{root.name}-{play.pattern}") or slug(root.name)
+    if base in taken:
+        base = slug(f"{base}-{play.playbook.removesuffix('.yml')}-{play.index}")
+    return _unique(base, taken)
 
 
 def migrate(root: Path, collections: Path | None = None, inventory: Path | None = None,
@@ -325,6 +335,7 @@ def migrate(root: Path, collections: Path | None = None, inventory: Path | None
 
     vocabulary = Vocabulary.default()
     fabrics: list[Fabric] = []
+    named: set[str] = set()
     for play in found:
         devices = frozenset(play.hosts)
         if not devices:
@@ -347,21 +358,78 @@ def migrate(root: Path, collections: Path | None = None, inventory: Path | None
             )
 
         fabric = Fabric(
-            name=_fabric_name(root, play, len(found) > 1),
+            name=_fabric_name(root, play, len(found) > 1, named),
             devices=tuple(sorted(devices)),
             inputs=_inputs(fragments, devices, vocabulary),
             play=play,
         )
         _fabric_name_of(fabric, inv.hostvars)
+        _report_play_vars(fabric, root, play)
         # Only now, with the translation proven faithful. Dropping anything
         # before the comparison above would weaken the one gate this module has.
         _report_unsupported(fabric, drop_descriptions)
         fabrics.append(fabric)
     return fabrics
 
 
+def _pooled_ids(design: dict) -> dict:
+    node_id = (design.get("fabric_numbering") or {}).get("node_id")
+    if isinstance(node_id, dict) and node_id.get("algorithm") == "pool_manager":
+        return node_id
+    return {}
+
+
+def _report_play_vars(fabric: Fabric, root: Path, play: Play) -> None:
+    """Variables set on the play itself, which no input XR carries.
+
+    ⚠ A source `ansible-inventory` cannot see, and one that changes the render:
+    `eos_designs-twodc-5stage-clos` runs eos_designs twice over the same hosts
+    and the second play sets `avd_digital_twin_mode: true`, producing a
+    different config into a different golden directory. Migrated without it, the
+    two fabrics come out identical and one of them is wrong.
+
+    **Reported, not carried.** Play vars outrank group and host vars, but the
+    oracle this migration checks itself against is per-host hostvars, which do
+    not include them -- so carrying them would silently weaken the one gate this
+    module has. Whoever migrates such a play adds the keys to an input by hand.
+    """
+    import yaml as _yaml
+
+    playbook = root / play.playbook
+    try:
+        document = _yaml.safe_load(playbook.read_text())
+    except (OSError, _yaml.YAMLError):
+        return
+    if not isinstance(document, list) or play.index > len(document):
+        return
+    variables = document[play.index - 1].get("vars") if isinstance(
+        document[play.index - 1], dict) else None
+    if isinstance(variables, dict) and variables:
+        fabric.notes.append(
+            f"{play.playbook} play #{play.index} sets {len(variables)} variable(s) on the "
+            f"play itself ({', '.join(sorted(variables))}); play vars outrank group and "
+            f"host vars and are NOT carried into any input"
+        )
+
+
 def _report_unsupported(fabric: Fabric, drop_descriptions: bool) -> None:
     """Note -- and optionally drop -- what pyavd will not honour."""
+    # State, not settings. An inventory already running `pool_manager` keeps its
+    # node IDs in a file AVD generated; this translates the *setting* and leaves
+    # the *assignments* behind. Applied to a fabric that is already deployed,
+    # that renumbers every device -- and a render reaches a switch as a full
+    # configuration replacement.
+    for inp in fabric.inputs:
+        pooled = _pooled_ids(inp.design)
+        if pooled:
+            where = pooled.get("pools_file") or "<root_dir>/intended/data/<fabric>-ids.yml"
+            fabric.notes.append(
+                f"node IDs come from a pool; its assignments live in {where} and do "
+                f"NOT travel with this migration. Seed them into the fabric "
+                f"(spec.nodeIdPool.seedConfigMapName) or every device is renumbered"
+            )
+            break
+
     if drop_descriptions:
         dropped = [
             f"{inp.name}.{path}"