Skip to content

feat(v2): MTV query inventory role - #75

Merged
tech2734 merged 1 commit into
redhat-cop:v2from
sabre1041:mtv_query_inventory_role
Aug 31, 2026
Merged

feat(v2): MTV query inventory role#75
tech2734 merged 1 commit into
redhat-cop:v2from
sabre1041:mtv_query_inventory_role

Conversation

@sabre1041

@sabre1041 sabre1041 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Description

New reusable role for querying MTV inventory

Type of Change

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation changes
  • style: Formatting, missing semi colons, etc; no code change
  • refactor: Refactoring production code
  • test: Adding missing tests, refactoring tests; no production code change
  • chore: Updating configs, etc; no production code change

How to invoke

- name: Query MTV providers
  ansible.builtin.import_role:
    name: infra.openshift_virtualization_migration.mtv_query_inventory
  vars:
    mtv_query_inventory_endpoint_path: /providers
    mtv_query_inventory_result_var: _mtv_providers

The invocation will query the /providers endpoint on the MTV inventory service and return the result in a variable called _mtv_providers`

@sabre1041
sabre1041 deployed to external-ci August 27, 2026 06:15 — with GitHub Actions Active
@sabre1041
sabre1041 changed the base branch from main to v2 August 27, 2026 06:15

@jeffcpullen jeffcpullen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some really pretty Ansible. I reviewed the code but did not test it. Left feedback and comments inline.

Higher level feedback:

  1. I don't see a changelog fragment in this PR.
  2. This leverages collection level variables but I don't see where those are defined at the main README or mentioned explicitly in the variable descriptions in this role.

Comment thread roles/mtv_query_inventory/meta/argument_specs.yml
Comment thread roles/mtv_query_inventory/meta/argument_specs.yml Outdated
Comment thread roles/mtv_query_inventory/meta/argument_specs.yml
Comment thread roles/mtv_query_inventory/meta/argument_specs.yml Outdated
Comment thread roles/mtv_query_inventory/meta/argument_specs.yml
Comment thread roles/mtv_query_inventory/tasks/main.yml
Comment thread roles/mtv_query_inventory/meta/argument_specs.yml Outdated
Comment thread roles/mtv_query_inventory/tasks/query_api.yml Outdated
Comment thread roles/mtv_query_inventory/tasks/query_exec.yml Outdated
Comment thread roles/mtv_query_inventory/vars/main.yml Outdated
Comment thread roles/mtv_query_inventory/meta/main.yml Outdated
Comment thread roles/mtv_query_inventory/defaults/main.yml
@sabre1041
sabre1041 force-pushed the mtv_query_inventory_role branch from 3952e44 to 9d63988 Compare August 27, 2026 19:38
@sabre1041
sabre1041 deployed to external-ci August 27, 2026 19:38 — with GitHub Actions Active
@sabre1041
sabre1041 force-pushed the mtv_query_inventory_role branch 2 times, most recently from 5e2c52c to 6e967ef Compare August 27, 2026 19:47
@sabre1041
sabre1041 deployed to external-ci August 27, 2026 19:58 — with GitHub Actions Active
tech2734
tech2734 previously approved these changes Aug 31, 2026

@tech2734 tech2734 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on homelab environment (Forklift 2.8.7 / OCP 4.14)

Ran both retrieval methods against the homelab MTV cluster with secure_logging: false to verify output.

api method

  • Route discovery worked — found forklift-inventory in openshift-mtv namespace
  • Constructed URL: https://forklift-inventory-openshift-mtv.apps.homelab-ocp.local.jmb
  • HTTP GET /providers returned 200 with full provider inventory

exec method

  • EndpointSlice lookup found the correct pod (forklift-controller-7599bf6845-bqkwf)
  • kubectl exec curl returned identical JSON to the api method

Results (both methods identical)

Type Provider Name
openshift host
vsphere vmware-target-1

Variable cascade

The defaults correctly resolve from generic inventory variables (openshift_host, openshift_temporary_api_key) — no extra role-specific vars needed in inventory.

Summary

  • Argument spec validation: ✅
  • api retrieval method: ✅
  • exec retrieval method: ✅
  • Result consistency across methods: ✅
  • Play recap: ok=18 changed=0 unreachable=0 failed=0 skipped=3 rescued=0 ignored=0

LGTM

Signed-off-by: Andrew Block <andy.block@gmail.com>
@tech2734

Copy link
Copy Markdown

Hey, I ran into an issue while integrating mtv_query_inventory into the new mtv_maps role (PR #80).

The endpoint path validation regex in tasks/main.yml:

mtv_query_inventory_endpoint_path is ansible.builtin.match('^/[a-zA-Z0-9_\\-/]*$')

rejects query parameters. The MTV inventory API requires ?detail=4 on datastore, storage class, network, and NAD queries to return the full object representation (including object.metadata.annotations which is needed for storage class default detection and NAD auto-matching):

/providers/vsphere/<uid>/datastores?detail=4
/providers/openshift/<uid>/storageclasses?detail=4
/providers/vsphere/<uid>/networks?detail=4
/providers/openshift/<uid>/networkattachmentdefinitions?detail=4

The regex also doesn't allow . which appears in UIDs.

Suggested fix — expand the regex to cover query parameters and dots:

mtv_query_inventory_endpoint_path is ansible.builtin.match('^/[a-zA-Z0-9_.\\-/]+(\\?[a-zA-Z0-9_.=&\\-]*)?$')

I worked around it locally for testing by patching the regex, and all 5 queries completed successfully against my homelab (Forklift 2.8.7).

@tech2734 tech2734 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM tested well

@tech2734
tech2734 merged commit fa9187e into redhat-cop:v2 Aug 31, 2026
20 of 21 checks passed
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.

3 participants