M #-: Refactor infra role / Introduce variants

[sk4zuzu]

• Replace frontend.xml.jinja template with mergeable structures
• Introduce variants: default, openvswitch, openvswitch_dpdk
• Add ipv4_mac filter
• Update README / Add inventory examples for each variant

[sk4zuzu]

This PR is based on ideas from #243, but it doesn't include CPU/NUMA isolation code. I think it stands on its own merits, regardless of the resource isolation problem is solved or not. 🤔

[rpetrescu-one]

Thanks for this, the fragment/variant approach is a clear improvement over the monolithic template. I re-rendered the Define Front-end VMs expression and context.sh.jinja with the branch defaults on ansible-core 2.16.19 (our pin) and 2.21.2, and diffed the output against origin/master for all three variants. The refactor is not behaviour-preserving yet: the documented default flow breaks in two places, and there are a few silent changes versus master (two of them undo #258 and #233). Details below, roughly by severity.

Blockers

1. context.sh.jinja L17 crashes when ETH0_SEARCH_DOMAIN is not set
{% if context.ETH0_SEARCH_DOMAIN %} boolean-tests a missing dict key, which raises under StrictUndefined. inventory/infra.yml, the README "default" example and the wiki inventory all omit that key, so "Create context.sh files" fails with 'dict object' has no attribute 'ETH0_SEARCH_DOMAIN' (both 2.16 and 2.21). Master used | d(""). Suggest {% if context.ETH0_SEARCH_DOMAIN | d("") %} (keeps "omit when unset or empty"), matching the is defined style used for ETH0_DNS on L14.

2. infra_xml_variant: "{{ undef() }}" (defaults L17) breaks every existing inventory
The old default was implicit (infra_bridge_type: bridge). inventory/infra.yml (which README L49 still points to) was not updated, so make I=inventory/infra.yml infra now fails at "Define Front-end VMs", after image download/clone and ISO creation, with just "Mandatory variable has not been overridden" and no variable name. The removed knobs (infra_bridge_type, infra_dpdk_socket_path, cputune_shares*) are silently ignored: an inventory that keeps infra_bridge_type: openvswitch_dpdk and adds infra_xml_variant: default to get past the error ends up with a plain bridge NIC. Suggest defaulting to default (or at least undef(hint='...')), an early assert rejecting the removed variables, and updating inventory/infra.yml.

3. xmltodict is missing from packaging/pyproject.toml
ansible.utils.from_xml/to_xml run on the controller and need it. It is in requirements.txt and the hatch env but not in packaging/pyproject.toml/uv.lock; a clean uv sync --frozen env fails all variants with Missing required library xmltodict (the old template worked there). Before this PR only the SUSE wicked path needed it. Please add it and refresh the lock. Related: from_xml only returns a dict since ansible.utils 5.1.0, and requirements.yml/galaxy.yml have no minimum version; roles/infra/meta/main.yml and the README "Dependencies" section should also list ansible.utils.

Silent behaviour changes vs master

4. Bridge/OVS NICs lost <driver name='vhost' queues='{{ vcpu_static }}'/> and <target dev='{{ frontend }}-nic0'/> (defaults L70 and L86)
#258 added the driver line for all NIC types; here it survives only in the openvswitch_dpdk fragment (L105). With vcpu_static > 1 the FE NIC silently falls back to single-queue virtio-net, and the tap device becomes vnetN instead of <frontend>-nic0. Both lines should be carried over.

5. <memoryBacking><access mode='shared'/></memoryBacking> is now emitted unconditionally (defaults L52)
Master emitted no memoryBacking for plain bridge VMs, memfd + shared for virtiofs, hugepages + shared for DPDK. With shared access and no memfd/hugepages, libvirt (qemuBuildMemoryBackendProps) selects memory-backend-file with a mem-path under memory_backing_dir (default /var/lib/libvirt/qemu/ram), i.e. every FE deployed with the default variant now has its whole RAM backed by a regular file on the hypervisor disk, and virtiofs users lose the memfd backend. Suggest emitting the block only when virtiofs is used (with <source type='memfd'/>), as before.

6. vcpu_shares: 200 no longer scales with vcpu_static (defaults L15)
#233 computed vcpu_static * cputune_shares_base; with 4 vCPUs shares drop from 800 to 200. If the flat default is intentional it should be stated in the PR/README; otherwise vcpu_shares: "{{ (vcpu_static | int) * 200 }}". The cap could be <shares>{{ [vcpu_shares | int, 10000] | min }}</shares> instead of the if/else with the literal twice.

7. The merge is fragile for custom fragments (deploy.yml L106-117)
xmltodict yields None/dict/list for 0/1/N children and combine(list_merge='append_rp') only appends when both sides are lists. Verified with one extra fragment appended to infra_xml.default: a fragment with a single <disk> replaces the base disk list (cdrom and root disk gone), two fragments each with one <interface> merge into one hybrid NIC, and a <features/> fragment nulls the subtree (<acpi/> dropped) because _fixup only covers cputune/devices. Nothing fails in Ansible; the VM just boots wrong. The shipped variants avoid this only because the base has no <interface>/<filesystem>. Given "mergeable structures" is the point of the PR, this needs either a force_list parse (small collection filter) or a clear note that fragments must not contribute single instances of repeatable elements. Also the _fixup NOTE is inaccurate: the merge does not error, it silently replaces devices with null; guarding the <devices> block in infra_xml_filesystems with {% if passthrough_fs %} makes _fixup unnecessary (byte-identical output).

8. dpdk_socket_path: null (defaults L10) removes the fail-fast
Master failed with 'infra_dpdk_socket_path' is undefined; now the dpdk variant renders <source type='unix' path='' mode='server'/>, define succeeds and only start fails, and since main.yml skips already-defined VMs the broken domain persists across re-runs. Suggest an assert: dpdk_socket_path is truthy before "Define Front-end VMs" when infra_xml_variant == 'openvswitch_dpdk', and a README note about the rename.

9. Lost truthiness guards
infra_vlan_id is defined (L73/L90/L107) was is defined and infra_vlan_id; infra_vlan_id: '' or null now renders <vlan><tag id=""/></vlan>, which libvirt rejects. Same for fs.driver_type is defined (L124): driver_type: ''/null gives <driver type=""/>.

Docs

• README L47: START_SCRIPT_BASE64 is documented as "(base64-encoded)", but the template applies | b64encode, so following the doc double-encodes the script.
• README L60+: the three examples are one YAML block with duplicate top-level keys (copied as-is, Ansible warns and keeps only the dpdk variant). They also use the private http://10.2.11.1/... image URL instead of the CloudFront one already in defaults, and diverge from each other (ETH0_SEARCH_DOMAIN only in two of three). The PR description says "Add inventory examples for each variant" but nothing was added under inventory/; I'd rather update inventory/infra.yml and add per-variant files there, and keep the README short.
• README table: infra_xml, infra_xml_base, infra_xml_cputune, infra_xml_memoryBacking, infra_xml_interfaces, infra_xml_filesystems are user-overridable but undocumented; infra_xml_variant (L34) lost the list of valid values the old infra_bridge_type row had, and "Defined which" -> "Defines which"; passthrough_fs (L24) still says "(check below)" but the only example was removed; vcpu_shares is typed str but used as int.

Nits / optional

• The {% set output = {} %} / output.update(...) loop can be a plain filter chain: infra_xml[infra_xml_variant] | map('ansible.utils.from_xml') | map('combine', _fixup, recursive=true) | combine(recursive=true, list_merge='append_rp') | ansible.utils.to_xml(full_document=false) (byte-identical for all three variants).
• The three infra_xml_interfaces fragments repeat the same <mac>/<vlan>/<model>/<alias> body; a shared NIC-body fragment plus a per-variant delta merges to the same dict and would have prevented item 4.
• ipv4_mac works and is stricter than the old inline expression (rejects leading zeros and 256), which is fine; the fmt parameter is never used and could go.
• dpdk_socket_path is a per-FE resource read from the hypervisor scope, so two FEs on the same HV get the same socket (pre-existing, but now documented that way). Either note "one FE per HV" for this variant or prefer hostvars[frontend].dpdk_socket_path.

What looks good: the openvswitch_dpdk variant renders XML identical to master, no None leaks from the loop on either core version, single/multiple <filesystem> entries round-trip correctly, and ansible-doc for the new filter is clean.

[sk4zuzu]

Thanks.

1. context.sh.jinja L17 crashes when ETH0_SEARCH_DOMAIN is not set

That's a valid one. 👍

2. infra_xml_variant: "{{ undef() }}" (defaults L17) breaks every existing inventory

Could not care less, for me it's no fix here. 👎

3. xmltodict is missing from packaging/pyproject.toml

That should come in another PR, no fix here. 👎

4. Bridge/OVS NICs lost and (defaults L70 and L86)

That's completely intended, no fix. 👎 #240

5. is now emitted unconditionally (defaults L52)

Unless this somehow matters no fix. It should work that way as well. 👎

6. vcpu_shares: 200 no longer scales with vcpu_static (defaults L15)

Can do either way. I think absolute value is better. 👎

7. The merge is fragile for custom fragments (deploy.yml L106-117)

I will debug more, looks like a valid-ish concern. 👍

8. dpdk_socket_path: null (defaults L10) removes the fail-fast

This is precisely intended, we can do an assertion though. 👍

9. Lost truthiness guards

I don't really see any problem here. 👎

From the "Nits"

dpdk_socket_path is a per-FE resource read from the hypervisor scope, so two FEs on the same HV get the same socket (pre-existing, but now documented that way). Either note "one FE per HV" for this variant or prefer hostvars[frontend].dpdk_socket_path.

This one has some merit, and I considered improving this, but decided I could not care less. 👎 🤗

[Aletibazo]

Found same blockers @rpetrescu-one mentions above. I would suggest using default as the default setting for infra_xml_variant as well.

[sk4zuzu]

@Aletibazo We can do that if you want, but I intended this "undef()" as something to force users to set it and I don't care about existing inventories in this specific case.

But let's give it a try. 👍😇

[sk4zuzu]

Regarding this nice piece of LLM slop 👍😍

7. The merge is fragile for custom fragments (deploy.yml L106-117)
   xmltodict yields None/dict/list for 0/1/N children and combine(list_merge='append_rp') only appends when both sides are lists. Verified with one extra fragment appended to infra_xml.default: a fragment with a single replaces the base disk list (cdrom and root disk gone), two fragments each with one merge into one hybrid NIC, and a fragment nulls the subtree ( dropped) because _fixup only covers cputune/devices. Nothing fails in Ansible; the VM just boots wrong. The shipped variants avoid this only because the base has no /. Given "mergeable structures" is the point of the PR, this needs either a force_list parse (small collection filter) or a clear note that fragments must not contribute single instances of repeatable elements. Also the _fixup NOTE is inaccurate: the merge does not error, it silently replaces devices with null; guarding the block in infra_xml_filesystems with {% if passthrough_fs %} makes _fixup unnecessary (byte-identical output).

It looks like LLM saw words "mergeable" and "append_rp", then generated response strictly according to that (missing the point). The problem is that the "fix" it confidently suggested (the "force_list small collection filter") is a nonsensical suggestion as it cannot possibly fix anything. It is impossible to acurately merge lists of dictionaries with "list_merge=append_rp", it's a contradiction.

What could work is a recursive strategic merge, something like this (we use it already for similar purpose elsewhere):

def combine(*args, merge_lists: false)
    recurse = proc { |a, b|
        case
        when a.is_a?(Hash) && b.is_a?(Hash)
            a.merge(b) { |_, a, b| recurse.call(a, b) }

        when merge_lists && a.is_a?(Array) && b.is_a?(Array)
            0.upto([a.size, b.size].max - 1).each_with_object([]) do |i, acc|
                [a, b].each_with_object([]) do |v, ab|
                    begin ab << v.fetch(i); rescue IndexError; end
                end.then do |ab|
                    acc << ((ab.size == 2) ? recurse.call(*ab) : ab[0])
                end
            end

        when merge_lists && a.is_a?(Hash) && b.is_a?(Array) && b[0].is_a?(Hash)
            [recurse.call(a, b[0]), *b[1..]]

        when merge_lists && a.is_a?(Array) && b.is_a?(Hash) && a[0].is_a?(Hash)
            [recurse.call(a[0], b), *a[1..]]

        else
            b
        end
    }

    ab = []

    while !(a = args.shift).nil?
        next if (ab << a).size < 2

        ab.replace [recurse.call(*ab)]
    end

    ab.first
end

But I believe this is a complete overkill for this simple role (that should have never been added to one-deploy in the first place IMHO).

a fragment with a single replaces the base disk lis

LLM missed what the design is all about, this particular case has to be handled differently, ie. all "disks" would have to be moved out of the base XML to a dedicated chunk and then "composed" into the final document.

I guess my mistake was naming it "mergeable" instead of "composable" + "append_rp" in this case is simply pointless. 🤗