[sabre1041]

Description

Base testing framework.

The following activities will be currently performed and driven by a playbook named vmf_e2e_testing.yml in the playbooks directory

1. Deploy an instance of AAP
2. Seed the deployed instance
3. Create an Namespace in OpenShift to execute the tests against
   1. Invoke Job Templates in AAP to perform the following
   2. Configure Registry credentials for the VDDK. This assumes that the username / password fields are provided alongside the VDDK image associated with the VMware environment
   3. Configure MTV Providers
   4. Configure MTV Maps
   5. Configure an MTV Plan for the Migration
   6. Trigger the migration
   7. Archive the Plan
   8. Delete the Plan

What is not currently covered:

• Failures
• Cleaning up resources

Prerequisites

The following should be provided beforehand

1. An OpenShift environment with the prerequisite Operators installed
2. A VMware environment to migrate VMs from

How to Test

This models itself off of the existing inventory that is provided in this project. There is a larger vision for the ways that e2e can/will be used. However, for the purpose of this PR, the following can be utilized

1. Define a migration_aap host group. Add in any variables that you need to add. The hosts can be left blank as it will dynamically be added at runtime
2. Define a vm_sources group and add in the applicable VMware environment details
3. Define a migration_clusters host group with the following
   1. Authentication details for the OpenShift clusters
   2. Define the vm_sources property and populate it with the hosts defined in the vm_sources group
   3. Define the aap_hosts. The value should be the name of the OpenShift host cluster prefixed with aap_. If the OpenShift cluster is called migration_cluster as the host, the value in this field should be aap_migration_cluster
   4. Define an extra variable called e2e_testing_openshift_plans_request to represent the Plans request that will be made by the framework. The majority of the values are defaulted, so all you need to do is add the VM's that are desired to be migrated. Here is an example

e2e_testing_openshift_plans_request:
  vms:
    - name: haproxy-user1

Invoke the vmf_e2e_testing.yml and try it out!

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

[vvaldez]

@sabre1041 Dual code review (secops + devops, independent passes) of this draft. Two blocking items, then coaching items. Verdict: NOT READY to un-draft yet — the BLOCKs are small fixes.

Blocking Findings

[BLOCK] roles/e2e_testing/tasks/run_openshift_cluster_vm_source.yml:3-13 — Verify Migration Request assert fires on the default value.
e2e_testing_openshift_plans_request defaults to {} (defaults/main.yml:62), so | default('', true) | length > 0 is false and the assert aborts every invocation of this entry point — including the registry-credentials-only path the file is structured to support. The Configure Plans block (line 47) was deliberately written to skip when empty; the assert contradicts it.
Fix: drop the assert (rely on the block's when:), or make the request truly required (argument spec required: true) and delete the when: guard. Pick one contract.

[BLOCK] roles/e2e_testing/defaults/main.yml:38-42 + meta/argument_specs.yml:39-46 — e2e_testing_verify_ssl type mismatch / silent TLS-off.
A bool-typed option is fed by a raw env string through a >- folded Jinja block, and the README contradicts itself (line 81: Type: bool, line 110: Type: str). Worse, there is no coupling check between verify_ssl=false and ca_cert_path: an operator env of K8S_AUTH_VERIFY_SSL=false + a stray K8S_AUTH_SSL_CA_CERT silently disables certificate verification against the API endpoint. For a tool that drives real migrations, silent TLS-off is blocking.
Fix: coerce with | bool at the end of the cascade; add an assert before the first k8s call in namespace_prepare.yml rejecting the ambiguous pair; regenerate the README so both sections say bool.

Coaching Items (pre-submission — no issues filed)

[HIGH] Destructive-by-default, no guardrail — playbooks/vmf_e2e_testing.yml:67-81 + run_openshift_cluster_vm_source.yml:91-141. A successful run migrates real VMs, then archives and deletes the plan (e2e_testing_archive_plan/e2e_testing_delete_plan both default true). Target is chosen purely by inventory wiring; a wrong K8S_AUTH_HOST or aap_hosts entry runs the whole flow against the wrong cluster, and --check does not stop wait: true job launches. Suggest: default archive/delete to false; add an explicit opt-in or target-allowlist assert (e2e_testing_openshift_host must be in a sanctioned list) before namespace_prepare.

[HIGH] aap_password in hostvars via add_host — playbooks/vmf_e2e_testing.yml:21-31. add_host binds the AAP password (and username) as persistent hostvars on the dynamic aap_* hosts for the rest of the run — in scope for every subsequent play, including import_playbook: vmf_seed_aap.yml and the transitive task graph of infra.aap_configuration.controller_job_launch (an external collection). Any hostvars rendering there leaks the password. Suggest: read the credential at task time from its original source instead of add_host, or at minimum audit controller_job_launch for hostvars rendering and scope no_log.

[MEDIUM] Registry creds in AAP extra_vars — roles/e2e_testing/tasks/configure_vddk_credentials.yml:31-32. AAP persists extra_vars on the Job record (visible to all org members with job-read), so the secret's lifetime extends to job-record retention. I verified the downstream utility_registry_credentials role does no_log on its k8s task, so log exposure is mitigated. Suggest: prefer an AAP credential for registry auth; if extra_vars are kept, document the exposure.

[MEDIUM] Hardcoded AAP job-template display names — roles/e2e_testing/vars/main.yml:3-7. Display name → job template is the dispatch trust boundary for destructive launches. Note: infra.aap_configuration is a pre-existing dependency (used by mtv_hook_invoke_aap) and is currently commented out in galaxy.yml — so this isn't new, but it becomes load-bearing here. Suggest: launch by ID or assert org+name resolution; document that names must match the seeded AAP (likely via aap_seed).

[MEDIUM] Zero tests — for a role that drives real cluster migrations. The env-var cascades in defaults/main.yml are pure Jinja and trivially unit-testable; a mock of controller_job_launch could assert the job structure. Suggest: add cascade unit tests + a mock-based job-structure test in this PR; document the manual E2E procedure in the README.

Notes

• roles/e2e_testing/tasks/main.yml is an empty entry point — include_role without tasks_from is a silent no-op. Add a comment or make it a real orchestrator.
• playbooks/vmf_e2e_testing.yml:41-47 vs :61-63 — the two plays test opposite directions of the aap_hosts relationship. One is likely wrong; verify how aap_hosts is populated.
• playbooks/vmf_e2e_testing.yml:18-31 — if the resolved utility_clusters group has no members, play 3 silently matches zero hosts and migration_aap stays empty; downstream plays no-op. Assert the group is non-empty after play 1.
• PR title feat(v2): E2E testing — scope v2 isn't in the documented scope list (roles/playbooks/modules/ci/deps); Tier-1 title validation may warn.
• configure_vddk_credentials.yml:26-30 — vddk.image | split('/') | first yields the image name, not a registry host, when image lacks a registry prefix.
• rfc1123 name derivation: the filter (plugins/filter/rfc1123.py) replaces invalid chars, strips, dedupes, lowercases, truncates to 63 — invalid names and most collisions are impossible; only two distinct names sharing a 63-char prefix would collide. Residual risk low.
• ~40 lines of repeated credentials:/organization: across five controller_job_launch blocks — consider factoring into a shared var.
• roles/e2e_testing/tests/inventory has no test playbook; files/.keep/templates/.keep are scaffolding noise.

Summary

BLOCK: 2 · HIGH: 2 · MEDIUM: 3 · NOTE: 8 · Files reviewed: 16

The draft is well-structured and follows collection conventions (docsible README, argument specs, vars/defaults split). The two BLOCKs are small fixes; the HIGH coaching items (destructive-tail guardrail, password-in-hostvars) are what will matter most for a fork-contributed PR that drives real migrations.

[sabre1041]

@sabre1041 Dual code review (secops + devops, independent passes) of this draft. Two blocking items, then coaching items. Verdict: NOT READY to un-draft yet — the BLOCKs are small fixes.

Blocking Findings

[BLOCK] roles/e2e_testing/tasks/run_openshift_cluster_vm_source.yml:3-13 — Verify Migration Request assert fires on the default value. e2e_testing_openshift_plans_request defaults to {} (defaults/main.yml:62), so | default('', true) | length > 0 is false and the assert aborts every invocation of this entry point — including the registry-credentials-only path the file is structured to support. The Configure Plans block (line 47) was deliberately written to skip when empty; the assert contradicts it. Fix: drop the assert (rely on the block's when:), or make the request truly required (argument spec required: true) and delete the when: guard. Pick one contract.

[BLOCK] roles/e2e_testing/defaults/main.yml:38-42 + meta/argument_specs.yml:39-46 — e2e_testing_verify_ssl type mismatch / silent TLS-off. A bool-typed option is fed by a raw env string through a >- folded Jinja block, and the README contradicts itself (line 81: Type: bool, line 110: Type: str). Worse, there is no coupling check between verify_ssl=false and ca_cert_path: an operator env of K8S_AUTH_VERIFY_SSL=false + a stray K8S_AUTH_SSL_CA_CERT silently disables certificate verification against the API endpoint. For a tool that drives real migrations, silent TLS-off is blocking. Fix: coerce with | bool at the end of the cascade; add an assert before the first k8s call in namespace_prepare.yml rejecting the ambiguous pair; regenerate the README so both sections say bool.

Coaching Items (pre-submission — no issues filed)

[HIGH] Destructive-by-default, no guardrail — playbooks/vmf_e2e_testing.yml:67-81 + run_openshift_cluster_vm_source.yml:91-141. A successful run migrates real VMs, then archives and deletes the plan (e2e_testing_archive_plan/e2e_testing_delete_plan both default true). Target is chosen purely by inventory wiring; a wrong K8S_AUTH_HOST or aap_hosts entry runs the whole flow against the wrong cluster, and --check does not stop wait: true job launches. Suggest: default archive/delete to false; add an explicit opt-in or target-allowlist assert (e2e_testing_openshift_host must be in a sanctioned list) before namespace_prepare.

[HIGH] aap_password in hostvars via add_host — playbooks/vmf_e2e_testing.yml:21-31. add_host binds the AAP password (and username) as persistent hostvars on the dynamic aap_* hosts for the rest of the run — in scope for every subsequent play, including import_playbook: vmf_seed_aap.yml and the transitive task graph of infra.aap_configuration.controller_job_launch (an external collection). Any hostvars rendering there leaks the password. Suggest: read the credential at task time from its original source instead of add_host, or at minimum audit controller_job_launch for hostvars rendering and scope no_log.

[MEDIUM] Registry creds in AAP extra_vars — roles/e2e_testing/tasks/configure_vddk_credentials.yml:31-32. AAP persists extra_vars on the Job record (visible to all org members with job-read), so the secret's lifetime extends to job-record retention. I verified the downstream utility_registry_credentials role does no_log on its k8s task, so log exposure is mitigated. Suggest: prefer an AAP credential for registry auth; if extra_vars are kept, document the exposure.

[MEDIUM] Hardcoded AAP job-template display names — roles/e2e_testing/vars/main.yml:3-7. Display name → job template is the dispatch trust boundary for destructive launches. Note: infra.aap_configuration is a pre-existing dependency (used by mtv_hook_invoke_aap) and is currently commented out in galaxy.yml — so this isn't new, but it becomes load-bearing here. Suggest: launch by ID or assert org+name resolution; document that names must match the seeded AAP (likely via aap_seed).

[MEDIUM] Zero tests — for a role that drives real cluster migrations. The env-var cascades in defaults/main.yml are pure Jinja and trivially unit-testable; a mock of controller_job_launch could assert the job structure. Suggest: add cascade unit tests + a mock-based job-structure test in this PR; document the manual E2E procedure in the README.

Notes

• roles/e2e_testing/tasks/main.yml is an empty entry point — include_role without tasks_from is a silent no-op. Add a comment or make it a real orchestrator.
• playbooks/vmf_e2e_testing.yml:41-47 vs :61-63 — the two plays test opposite directions of the aap_hosts relationship. One is likely wrong; verify how aap_hosts is populated.
• playbooks/vmf_e2e_testing.yml:18-31 — if the resolved utility_clusters group has no members, play 3 silently matches zero hosts and migration_aap stays empty; downstream plays no-op. Assert the group is non-empty after play 1.
• PR title feat(v2): E2E testing — scope v2 isn't in the documented scope list (roles/playbooks/modules/ci/deps); Tier-1 title validation may warn.
• configure_vddk_credentials.yml:26-30 — vddk.image | split('/') | first yields the image name, not a registry host, when image lacks a registry prefix.
• rfc1123 name derivation: the filter (plugins/filter/rfc1123.py) replaces invalid chars, strips, dedupes, lowercases, truncates to 63 — invalid names and most collisions are impossible; only two distinct names sharing a 63-char prefix would collide. Residual risk low.
• ~40 lines of repeated credentials:/organization: across five controller_job_launch blocks — consider factoring into a shared var.
• roles/e2e_testing/tests/inventory has no test playbook; files/.keep/templates/.keep are scaffolding noise.

Summary

BLOCK: 2 · HIGH: 2 · MEDIUM: 3 · NOTE: 8 · Files reviewed: 16

The draft is well-structured and follows collection conventions (docsible README, argument specs, vars/defaults split). The two BLOCKs are small fixes; the HIGH coaching items (destructive-tail guardrail, password-in-hostvars) are what will matter most for a fork-contributed PR that drives real migrations.

Hi @vvaldez . Thanks for the feedback

• Corrected the e2e_testing_openshift_plans_request issue
• The argument_spec issue is a larger issue as it affects multiple roles. If it is an issue, we can create a issue to enable resolving holistically
• The rest are items that could be improved in the future

Let me know if theres anything blocking where we wouldnt be able to merge in this integration as the first step towards e2e coverage

[vvaldez]

@sabre1041 Ran this PR through a live staged E2E on a real environment (OCP 4.21 + vSphere 8.0.2 + AAP 2.6 on-cluster, 9 runs through the flow). Answering your question first: nothing blocking merge. The live run validated the flow end to end; findings below.

What the live run validated (final form)

• Your e2e_testing_openshift_plans_request fix is confirmed working: the assert caught the docs' key=value extra-var form (arrives as a string on core 2.16, never YAML-parsed), and the whole-JSON form ran green from then on.
• AAP job chain green idempotently: Provider -> Maps -> Plans.
• mtv_plans request processing: vms: [{name: ablock-test-rhel9}] resolved to MOID vm-21 on the plan CR; network/storage maps wired; targetNamespace correct.
• Plan reached Ready = True.
• Migrate action: migration launched, monitored, and the failed-VM condition detected loudly — Migration 'e2e-test' completed but did not succeed. Failed VMs: ablock-test-rhel9. That failure exit is exactly the behavior this PR should have.
• Disk copy: 2.15 GB via VDDK/nbdkit succeeded.

The migration itself failed in guest conversion — environment + upstream, not this PR:

1. The host CPU is Intel Xeon E5-2620 v2 (x86-64-v2 only). MTV 2.11/2.12 default to the rhel10-based mtv-virt-v2v-rhel10 v2v image on newer OCP; it requires x86-64-v3, so every validator/conversion pod dies with Fatal glibc error: CPU does not support x86-64-v3. Workaround for CPU-v2 targets: spec.virt_v2v_image_fqin on the ForkliftController CR — the operator ships both variants (relatedImages.virt_v2v_rhel9), and forcing the rhel9 image got the plan Ready plus the full disk copy.
2. With the rhel9 image, conversion failed at inspection: the MTV controller unconditionally appends --no-fstrim to virt-v2v-inspector (upstream PR 5867), and the rhel9 image's virt-v2v 2.7.1 does not know that flag. Upstream: Conversion error virt-v2v: virt-v2v dnf package outdated on CentOS Stream 9 Base Image kubev2v/forklift#5959 (open, no mitigation).

Findings from the live run (extends the earlier review)

[CHANGE] verify_ready passes vacuously when the plan query returns no results. roles/mtv_plans/tasks/verify_ready.yml:40-47: readiness is json_query(...) | length == results | length. Empty result list -> both sides 0 -> the loop exits green and the flow proceeds to migrate a nonexistent plan. Hit live: the plan CR was deleted under a running verify loop; the next poll saw no plans, the loop passed, and the migrate action launched against nothing. Guard with a non-empty check.

[CHANGE, minor] rescue path crashes on facts set later in the validate flow. When an early validate check fails in mtv_provider, the rescue task file dies with '_mtv_provider_provider_name' is undefined and the final failure message loses the real reason.

[CONFIRMED LIVE] scheme'd vddkInitImage passes silently. The split('/') | first note from the earlier review: a scheme'd image reached the provider CR and Forklift rejected it (VDDKInitImageUnavailable (InvalidImageName)) while the provider job stayed green — the failure surfaced two stages later in the plan wait loop. Validate scheme-less in mtv_provider before applying the CR.

[NOTE] lifecycle gaps worth documenting:

• aap_seed snapshots the OCP API token into the AAP target credential at seed time. The 24h token here expired mid-run and the AAP job's k8s calls started 401ing with no hint of the cause. Document the re-seed/refresh path after token rotation.
• aap_seed also snapshots the whole vddk dict into AAP inventory host vars at seed time — later inventory fixes silently never reach the provider CR (this is how the scheme'd image persisted through green provider jobs).
• mtv_migrate_verify_per_vm_status defaults off — a failed migration's per-VM error conditions never reach the AAP job output; the real reason was only visible in the conversion pod log.
• MTV 2.11/2.12 require x86-64-v3-capable compute (rhel10 v2v images) — a one-line prerequisites note would save the next person this diagnosis.

The two CHANGEs are small fixes in this PR. The argument_spec item you flagged affects multiple roles as you said — agree that one is an issue, not this PR.

[sabre1041]

@sabre1041 Ran this PR through a live staged E2E on a real environment (OCP 4.21 + vSphere 8.0.2 + AAP 2.6 on-cluster, 9 runs through the flow). Answering your question first: nothing blocking merge. The live run validated the flow end to end; findings below.

What the live run validated (final form)

• Your e2e_testing_openshift_plans_request fix is confirmed working: the assert caught the docs' key=value extra-var form (arrives as a string on core 2.16, never YAML-parsed), and the whole-JSON form ran green from then on.
• AAP job chain green idempotently: Provider -> Maps -> Plans.
• mtv_plans request processing: vms: [{name: ablock-test-rhel9}] resolved to MOID vm-21 on the plan CR; network/storage maps wired; targetNamespace correct.
• Plan reached Ready = True.
• Migrate action: migration launched, monitored, and the failed-VM condition detected loudly — Migration 'e2e-test' completed but did not succeed. Failed VMs: ablock-test-rhel9. That failure exit is exactly the behavior this PR should have.
• Disk copy: 2.15 GB via VDDK/nbdkit succeeded.

The migration itself failed in guest conversion — environment + upstream, not this PR:

1. The host CPU is Intel Xeon E5-2620 v2 (x86-64-v2 only). MTV 2.11/2.12 default to the rhel10-based mtv-virt-v2v-rhel10 v2v image on newer OCP; it requires x86-64-v3, so every validator/conversion pod dies with Fatal glibc error: CPU does not support x86-64-v3. Workaround for CPU-v2 targets: spec.virt_v2v_image_fqin on the ForkliftController CR — the operator ships both variants (relatedImages.virt_v2v_rhel9), and forcing the rhel9 image got the plan Ready plus the full disk copy.
2. With the rhel9 image, conversion failed at inspection: the MTV controller unconditionally appends --no-fstrim to virt-v2v-inspector (upstream PR 5867), and the rhel9 image's virt-v2v 2.7.1 does not know that flag. Upstream: Conversion error virt-v2v: virt-v2v dnf package outdated on CentOS Stream 9 Base Image kubev2v/forklift#5959 (open, no mitigation).

Findings from the live run (extends the earlier review)

[CHANGE] verify_ready passes vacuously when the plan query returns no results. roles/mtv_plans/tasks/verify_ready.yml:40-47: readiness is json_query(...) | length == results | length. Empty result list -> both sides 0 -> the loop exits green and the flow proceeds to migrate a nonexistent plan. Hit live: the plan CR was deleted under a running verify loop; the next poll saw no plans, the loop passed, and the migrate action launched against nothing. Guard with a non-empty check.

[CHANGE, minor] rescue path crashes on facts set later in the validate flow. When an early validate check fails in mtv_provider, the rescue task file dies with '_mtv_provider_provider_name' is undefined and the final failure message loses the real reason.

[CONFIRMED LIVE] scheme'd vddkInitImage passes silently. The split('/') | first note from the earlier review: a scheme'd image reached the provider CR and Forklift rejected it (VDDKInitImageUnavailable (InvalidImageName)) while the provider job stayed green — the failure surfaced two stages later in the plan wait loop. Validate scheme-less in mtv_provider before applying the CR.

[NOTE] lifecycle gaps worth documenting:

• aap_seed snapshots the OCP API token into the AAP target credential at seed time. The 24h token here expired mid-run and the AAP job's k8s calls started 401ing with no hint of the cause. Document the re-seed/refresh path after token rotation.
• aap_seed also snapshots the whole vddk dict into AAP inventory host vars at seed time — later inventory fixes silently never reach the provider CR (this is how the scheme'd image persisted through green provider jobs).
• mtv_migrate_verify_per_vm_status defaults off — a failed migration's per-VM error conditions never reach the AAP job output; the real reason was only visible in the conversion pod log.
• MTV 2.11/2.12 require x86-64-v3-capable compute (rhel10 v2v images) — a one-line prerequisites note would save the next person this diagnosis.

The two CHANGEs are small fixes in this PR. The argument_spec item you flagged affects multiple roles as you said — agree that one is an issue, not this PR.

@vvaldez Corrected the issues associated with verifying plans along with the credentials for the VDDK image. If everything looks satisfactory, please go ahead and merge

[vvaldez]

Delta re-review (827d425 -> 6a19227) — dual pass (secops + devops, independent). Both response fixes verified sound:

• verify_ready/verify_archived empty-results guard: the old expression evaluated 0 == 0 -> true with zero plans, declaring them ready/archived vacuously; the new length > 0 and (...) closes it. Real bug fix.
• Token extraction in utility_openshift_target_credential: wait + set_fact always run, display stays opt-in, no_log on the fact. Correct.
• VDDK registry-hostname assert: the when chain guarantees non-empty image/username/password before the assert evaluates, and the fail_msg echoes only the image reference, not the credentials from the same dict. Right call.

Approving. Two follow-ups for a future pass, neither blocking:

• The auto-generated cluster-admin token (SA + secret from the api-key generation path) has no TTL and no revocation path. Consider TokenRequest expirationSeconds, a least-privilege cluster role for the e2e path, or a teardown task before next release.
• The install_aap gate silently no-ops the whole flow for inventories that predate the variable (migration_aap ends empty, every downstream play skips). Consider a guard after the add_host play when the group is empty, plus a changelog fragment for the user-facing changes.