Skip to content

feat: support multiple storage map definitions#42

Open
albertoflorez wants to merge 1 commit into
redhat-cop:mainfrom
albertoflorez:feat/mtv-multiple-storage-maps
Open

feat: support multiple storage map definitions#42
albertoflorez wants to merge 1 commit into
redhat-cop:mainfrom
albertoflorez:feat/mtv-multiple-storage-maps

Conversation

@albertoflorez
Copy link
Copy Markdown

@albertoflorez albertoflorez commented May 26, 2026

Description

This PR enables the creation of multiple StorageMap resources within the same MTV management execution. It introduces a new variable mtv_management_multiple_storage_maps_overrides which allows users to define a dictionary of storage maps.

Motivation

In complex migration scenarios, different virtual machines or plans might require different storage mapping strategies (e.g., migrating to different StorageClasses based on performance tiers).

Currently, the role only supports creating a single, default storage map. By allowing multiple maps:

  • Administrators can pre-provision various storage mapping configurations.
  • Migration plans can be more agile by referencing the specific map that fits their needs.
  • It reduces the need for multiple role executions to create different infrastructure configurations.

Changes

  • mtv_maps.yml: Added logic to detect mtv_management_multiple_storage_maps_overrides. If defined, the role loops through the dictionary to create multiple maps. It maintains backward compatibility with the single-map approach if the new variable is empty or undefined.
  • storagemap.yml.j2: Updated the metadata name to be dynamic, using the key from the overrides dictionary or falling back to the default naming convention.
  • main.yml (defaults): Added the new dictionary structure with documentation examples.

Data Structure Example

mtv_management_multiple_storage_maps_overrides:
  gold-storage-map:
    - id: "datastore-1"
      storageClass: "ocs-storagecluster-ceph-rbd"
      include: true
    - id: "datastore-2"
      storageClass: "ocs-storagecluster-ceph-rbd"
      include: true
  silver-storage-map:
    - id: "datastore-2"
      storageClass: "slow-nfs-storage"

Testing

  • Verified that if mtv_management_multiple_storage_maps_overrides is provided, all specified maps are created in OpenShift.
  • Verified that the default single-map creation still works as expected when the new variable is not used.

@albertoflorez albertoflorez requested a review from sabre1041 as a code owner May 26, 2026 11:11
@stevefulme1
Copy link
Copy Markdown
Contributor

Good use case — being able to pre-provision multiple storage maps for different performance tiers is a real need in complex migration environments. The implementation is clean and small. A few things to flag:


1. Both variables defined = both single AND multiple maps silently skipped

If a user sets mtv_management_multiple_storage_maps_overrides (non-empty) and also has mtv_management_storage_map_overrides populated, the multiple-maps path runs but it overrides the mtv_management_storage_map_overrides variable via the loop vars: block:

vars:
  mtv_management_storage_map_overrides: "{{ item.value }}"

This is actually fine for the multiple path — each iteration gets its own overrides. But the single-map path is skipped entirely (the when excludes it when mtv_management_multiple_storage_maps_overrides | length > 0). This means any entries in the original mtv_management_storage_map_overrides are silently ignored.

This is probably the intended behavior, but it's not obvious from the defaults file that the two variables are mutually exclusive. A validation assertion or a comment in defaults/main.yml noting that mtv_management_multiple_storage_maps_overrides takes precedence and mtv_management_storage_map_overrides is ignored when the multiple path is active would prevent confusion.

2. Redundant inventory queries on each loop iteration

The "Configure MTV Storage Maps (Multiple)" task calls _mtv_storage_map.yml in a loop, and _mtv_storage_map.yml runs these queries every time:

  • Query destination StorageClasses
  • Query source datastores (vSphere or oVirt)
  • Determine default StorageClass

These are all re-queried on every iteration of the loop even though the results don't change between storage map definitions. For 2-3 maps this is fine, but at scale it's unnecessary API calls against the MTV inventory. Not a blocker, but worth noting as a potential optimization — the queries could be hoisted above the loop.

3. defined_storage_map_name is not cleaned up for the single-map path

The template now uses:

name: "{{ defined_storage_map_name | default(mtv_management_source_target + '-' + mtv_management_destination_target) | infra.openshift_virtualization_migration.rfc1123 }}"

In the single-map path, defined_storage_map_name is never set, so default() kicks in correctly. However, if the role is called twice in one play (first with multiple maps, then without), the defined_storage_map_name fact from the last loop iteration would leak into the single-map run. Same class of fact-leakage issue as PR #40. Low probability in practice, but could be guarded by clearing the fact at the top of _mtv_storage_map.yml:

- name: _mtv_storage_map | Clear storage map name override
  ansible.builtin.set_fact:
    defined_storage_map_name: ""

Although — since defined_storage_map_name is passed as a vars: on include_tasks, it shouldn't persist as a fact. Actually on second look, vars: on include_tasks is scoped to the included file, so this should be safe. Disregard this one unless you're also setting it via set_fact elsewhere.

4. Map key names are user-controlled K8s resource names

The dictionary keys (gold-storage-map, silver-storage-map, etc.) flow directly into metadata.name via the rfc1123 filter. The filter handles RFC 1123 normalization, so this is safe. Just worth noting in the docs that map key names will be sanitized — a key like Gold_Storage (Tier 1) will silently become something like gold-storage--tier-1- which might surprise users checking oc get storagemap.

5. Minor style nits

  • Double blank line at the end of the new block in defaults/main.yml (lines 166-167)
  • Trailing whitespace on the when: lines in mtv_maps.yml

Security

No concerns. No credentials, secrets, or user-injectable content in this PR. The storage map overrides are structured data (datastore IDs and StorageClass names) that flow into K8s resource definitions through the existing template pipeline.

Summary

Area Verdict
Format/style Pass (minor trailing whitespace, double blank line)
Backward compat Pass — defaults to {}, single-map path untouched when empty
Correctness (happy path) Pass
Correctness (mutual exclusivity) Works but undocumented — add a comment noting precedence
Security No issues

Solid feature, small and well-scoped. The main ask is just a note in defaults/main.yml clarifying the relationship between mtv_management_storage_map_overrides and mtv_management_multiple_storage_maps_overrides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants