Add virt-who Deploy Config Ansible REX job tests for all hypervisors - #22218
Draft
jnagare-redhat wants to merge 3 commits into
Draft
Add virt-who Deploy Config Ansible REX job tests for all hypervisors#22218jnagare-redhat wants to merge 3 commits into
jnagare-redhat wants to merge 3 commits into
Conversation
Add test coverage for the 'Deploy virt-who Config' Ansible REX job template across all hypervisor types (ESX, HyperV, KubeVirt, Libvirt, Nutanix) via both API and UI paths. Changes: - Add deploy_configure_by_job_api() and deploy_configure_by_job_ui() helper functions to robottelo/utils/virtwho.py - Add deploy_validation_on_host() for validating virt-who deployment on remote content hosts used by UI REX jobs - Add deploy_via_job_api and deploy_via_job_ui fixtures - Add test_positive_deploy_configure_by_job tests to all 10 hypervisor test files (5 API + 5 UI) - Simplify deploy_type_api and deploy_type_ui fixtures by removing indirect parametrize for the 'id' deploy type (only 'script' remains) - Remove unused get_configure_command() and get_configure_command_option() functions - Update hypervisor mapping detection to also match 'Sending updated Host-to-guest mapping to' log pattern Verifies: SAT-46996 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds virt-who Ansible REX job-based deployment coverage for all hypervisors, refactors virt-who deployment helpers to target a CDN-backed content host instead of Satellite/Capsule, and simplifies existing deploy fixtures by removing hammer command-based paths and unused helpers while updating log parsing for newer Satellite versions. Sequence diagram for API-based virt-who Ansible REX job deployment to content hostsequenceDiagram
participant TestRunner
participant VirtWhoUtils
participant Satellite as target_sat
participant ContentHost as content_host
participant CDN
TestRunner->>VirtWhoUtils: deploy_configure_by_job_api(form_data, hypervisor_type, org, activation_key, target_sat, content_host, location)
VirtWhoUtils->>VirtWhoUtils: virtwho_cleanup()
VirtWhoUtils->>VirtWhoUtils: _setup_virtwho_guest(hypervisor_type, org, activation_key, target_sat)
VirtWhoUtils->>ContentHost: _register_content_host_with_virtwho(content_host, target_sat, org, activation_key, location)
ContentHost->>CDN: register_to_cdn()
ContentHost->>CDN: enable_repo(aps_repo, force=True)
ContentHost->>CDN: execute(dnf install -y virt-who)
VirtWhoUtils->>Satellite: JobTemplate().search(query)
VirtWhoUtils->>Satellite: JobInvocation().run(data={job_template_id, inputs, search_query})
VirtWhoUtils->>Satellite: wait_for_tasks(resource_type = JobInvocation, resource_id = job[id])
VirtWhoUtils->>Satellite: JobInvocation(id=job[id]).read()
Satellite-->>VirtWhoUtils: result.succeeded
VirtWhoUtils->>ContentHost: execute(rm -f /var/log/rhsm/rhsm.log)
VirtWhoUtils->>ContentHost: execute(systemctl restart virt-who; sleep 10)
alt debug
VirtWhoUtils->>VirtWhoUtils: deploy_validation_on_host(hypervisor_type, content_host)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
deploy_validation_on_host, the wait loop repeatedly runshost.execute('cat /var/log/rhsm/rhsm.log'); consider reading the log once perwait_foriteration and reusing it both for the condition and the subsequent parsing to avoid duplicate remote calls. - The job invocation helpers (
deploy_configure_by_job_apianddeploy_configure_by_job_ui) hard-code strings likeDeploy virt-who Configand the job description; centralizing these names/constants would reduce the chance of breakage if the template name or description changes. - There is now duplicated logic for virt-who deployment validation in
deploy_validationanddeploy_validation_on_host; you could extract common mapping-parsing code into a shared helper to make future changes easier and keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `deploy_validation_on_host`, the wait loop repeatedly runs `host.execute('cat /var/log/rhsm/rhsm.log')`; consider reading the log once per `wait_for` iteration and reusing it both for the condition and the subsequent parsing to avoid duplicate remote calls.
- The job invocation helpers (`deploy_configure_by_job_api` and `deploy_configure_by_job_ui`) hard-code strings like `Deploy virt-who Config` and the job description; centralizing these names/constants would reduce the chance of breakage if the template name or description changes.
- There is now duplicated logic for virt-who deployment validation in `deploy_validation` and `deploy_validation_on_host`; you could extract common mapping-parsing code into a shared helper to make future changes easier and keep behavior consistent.
## Individual Comments
### Comment 1
<location path="robottelo/utils/virtwho.py" line_range="370-375" />
<code_context>
+ mapping.append(_parse_entry(entry))
+ mapping = [m for m in mapping if m is not None]
+ hypervisor_name = None
+ for item in mapping[-1]['hypervisors']:
+ for guest in item['guestIds']:
+ if guest_uuid in guest['guestId']:
+ hypervisor_name = item['hypervisorId']['hypervisorId']
+ break
+ if not hypervisor_name:
+ raise VirtWhoError(f'Failed to get hypervisor_name for guest {guest_name}')
+ for h in Host.list({'search': hypervisor_name}):
</code_context>
<issue_to_address>
**issue:** Handle the case where no mapping entries are parsed to avoid IndexError.
After filtering out `None` entries, `mapping` may still be empty. Accessing `mapping[-1]['hypervisors']` in that case will raise `IndexError` (or `TypeError` if the last entry is malformed) before you raise `VirtWhoError`. Add a guard (e.g., `if not mapping: raise VirtWhoError(...)`) before indexing, or otherwise ensure you only access `mapping[-1]` when a valid entry is present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+370
to
+375
| for item in mapping[-1]['hypervisors']: | ||
| for guest in item['guestIds']: | ||
| if guest_uuid in guest['guestId']: | ||
| hypervisor_name = item['hypervisorId']['hypervisorId'] | ||
| break | ||
| if not hypervisor_name: |
Contributor
There was a problem hiding this comment.
issue: Handle the case where no mapping entries are parsed to avoid IndexError.
After filtering out None entries, mapping may still be empty. Accessing mapping[-1]['hypervisors'] in that case will raise IndexError (or TypeError if the last entry is malformed) before you raise VirtWhoError. Add a guard (e.g., if not mapping: raise VirtWhoError(...)) before indexing, or otherwise ensure you only access mapping[-1] when a valid entry is present.
for more information, see https://pre-commit.ci
jnagare-redhat
marked this pull request as draft
July 22, 2026 13:10
…lite Remove capsule dependency from deploy_configure_by_job_ui and align both API and UI deploy functions to target a content host. Extract shared setup into _setup_virtwho_guest and _register_content_host_with_virtwho helpers to eliminate duplication across all three deploy functions. The content host registers to CDN for virt-who package availability before registering to Satellite for REX job targeting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jnagare-redhat
added
No-CherryPick
jnagare-redhat
marked this pull request as ready for review
July 22, 2026 17:32
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In deploy_validation_on_host the wait_for predicate re-executes
cat /var/log/rhsm/rhsm.logon every check and then reads the file again afterward; consider capturing the log content once per poll and reusing it to avoid repeated remote commands and make the mapping parsing more efficient. - The new Ansible job helpers mix
assert-based error reporting with VirtWhoError exceptions (e.g. in_register_content_host_with_virtwhovs other functions); standardizing on explicit exception types would make failures easier to interpret and more consistent with the rest of the virt-who utilities. - The UI job flow relies on a hard-coded
job_descriptionstring ('Deploy virt-who configuration virt-who-config') to locate the JobInvocation; using a more robust identifier (e.g. template name plus host or ID from the scheduling call) would reduce brittleness if the job template description changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In deploy_validation_on_host the wait_for predicate re-executes `cat /var/log/rhsm/rhsm.log` on every check and then reads the file again afterward; consider capturing the log content once per poll and reusing it to avoid repeated remote commands and make the mapping parsing more efficient.
- The new Ansible job helpers mix `assert`-based error reporting with VirtWhoError exceptions (e.g. in `_register_content_host_with_virtwho` vs other functions); standardizing on explicit exception types would make failures easier to interpret and more consistent with the rest of the virt-who utilities.
- The UI job flow relies on a hard-coded `job_description` string ('Deploy virt-who configuration virt-who-config') to locate the JobInvocation; using a more robust identifier (e.g. template name plus host or ID from the scheduling call) would reduce brittleness if the job template description changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
Contributor
Author
|
Collaborator
|
PRT Result |
1 task
Contributor
Author
|
trigger: test-robottelo |
Contributor
Author
|
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
PRT-Passed
jnagare-redhat
marked this pull request as draft
July 28, 2026 15:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_setup_virtwho_guestand_register_content_host_with_virtwho) to eliminate duplication across all three deploy functions (API job, UI job, script)register_sat_and_enable_aps_repodependency from job-based deploy fixtures since content hosts now get virt-who from CDN directlyVerifies: SAT-46996
Airgun PR: SatelliteQE/airgun#2488
Test plan
pytest tests/foreman/virtwho/api/test_esx_sca.py -k test_positive_deploy_configure_by_jobpytest tests/foreman/virtwho/ui/test_esx_sca.py -k test_positive_deploy_configure_by_jobpytest tests/foreman/virtwho/api/test_esx_sca.py -k test_positive_deploy_configure_by_script🤖 Generated with Claude Code
Summary by Sourcery
Add new coverage for deploying virt-who configuration via the Ansible REX job against a content host, and refactor shared virt-who deployment logic to support all hypervisor types.
New Features:
Enhancements:
Tests:
Chores: