WIP: [Storage] Windows tests should all use vTPM - module CDI Clone#4571
WIP: [Storage] Windows tests should all use vTPM - module CDI Clone#4571acinko-rh wants to merge 19 commits intoRedHatQE:mainfrom
Conversation
Co-Authored: Claude Code Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/wip |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces a parametrized HTTP-based Windows DV clone test with a new tier3 test that builds a VM from a registry-backed cloned DV (Windows 2022) including vTPM. Adds fixtures to provision source and cloned DataVolumes from a registry and introduces a Windows 2K22 preference constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
acinko-rh
changed the title
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
josemacassan and
kshvaika
openshift-virtualization-qe-bot
requested review from
Ahmad-Hafe,
jpeimer and
kgoldbla
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/storage/utils.py (1)
541-584:⚠️ Potential issue | 🟠 Major[HIGH] Landing a helper already labeled "DEBUG ONLY - MARKED FOR DELETION" contradicts the dead-code policy.
The docstring and the trailing
# TODO: Remove - debug onlyon Line 541 announce this function is scaffolding, yet it's imported and exercised bytest_successful_vm_from_cloned_dv_windows_with_vtpmintests/storage/cdi_clone/test_clone.py. Either the verification is part of the intended coverage (then drop the "debug only" labeling and keep it properly) or it isn't (then don't merge it). Shipping code pre-flagged for deletion is how dead code quietly permanents itself in the repo.As per coding guidelines: "No dead code in Python. Every function, variable, fixture MUST be used or removed."
✂️ Suggested cleanup
-def verify_vtpm_in_windows_vm(vm, admin_client): # TODO: Remove - debug only +def verify_vtpm_in_windows_vm(vm, admin_client): """ Verify vTPM is properly configured and detected in a Windows VM. - **DEBUG ONLY - MARKED FOR DELETION** - Performs two-layer validation:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/utils.py` around lines 541 - 584, The helper verify_vtpm_in_windows_vm is labelled "DEBUG ONLY - MARKED FOR DELETION" yet is referenced by test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code marker and make the function a first-class test utility: delete the "# TODO: Remove - debug only" comment, update the docstring to reflect it is an intentional test helper (remove "DEBUG ONLY"), and ensure any module exports/imports remain correct so tests continue to import verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it, remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and delete the function instead.tests/storage/cdi_clone/test_clone.py (1)
1-3:⚠️ Potential issue | 🟡 Minor[MEDIUM] New feature test requires an STP (or RFE/Jira epic) link in the module docstring.
The module docstring is still just
"""Clone tests""", but this PR adds a new feature test (test_successful_vm_from_cloned_dv_windows_with_vtpm) covering vTPM on cloned Windows DVs. As per coding guidelines: "Every new feature test module MUST include an STP link in the module docstring. If there is no STP, the module docstring MUST include a link to the RFE or Jira epic (not support cases) for coverage tracking." The PR description also has an empty Jira ticket field, which needs to be filled in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cdi_clone/test_clone.py` around lines 1 - 3, The module docstring currently just says "Clone tests" but this PR adds the new feature test test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module docstring to include the required STP link (or, if none, the RFE/Jira epic URL) for coverage tracking and also populate the empty Jira ticket field in the PR description; modify the top-of-file docstring in the test module to include a one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside the existing summary so automated checks and reviewers can verify the traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Line 248: The test contains a placeholder Polarion ID in the pytest marker:
replace pytest.mark.polarion("CNV-XXXXX") in
tests/storage/cdi_clone/test_clone.py with the real Polarion/Jira ID for this
test (use the official ticket string), and update the PR description’s empty
Jira ticket field to the same ID so reporting and traceability are correct;
ensure the marker value is a non-placeholder string and re-run the
test/reporting to verify the ID appears properly.
- Around line 72-110: Move the helper
create_windows_vm_with_vtpm_validate_guest_agent_info out of the test module
into tests/storage/utils.py (next to
create_windows_vm_validate_guest_agent_info), make admin_client a required
positional argument (remove the =None default) so vTPM validation always runs,
relocate the imports to the top of the new module (import
validate_os_info_vmi_vs_windows_os from utilities.ssp, wait_for_windows_vm from
utilities.virt, and verify_vtpm_in_windows_vm from tests.storage.utils or the
correct canonical module), and remove the runtime None check (do not gate
verify_vtpm_in_windows_vm with if admin_client; call it directly). Ensure any
remaining None comparisons use identity (if x is None) when applicable.
- Line 103: The test creates the VM in a halted state
(VirtualMachine.RunStrategy.HALTED) so wait_for_windows_vm(vm=vm, ...) fails
because it immediately runs SSH; call running_vm(vm=vm) to transition the VM to
Running before calling wait_for_windows_vm. Insert a running_vm(vm=vm)
invocation inside the context manager just prior to wait_for_windows_vm
(mirroring create_windows19_vm) so the VM is started and SSH connectivity is
available.
---
Outside diff comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Around line 1-3: The module docstring currently just says "Clone tests" but
this PR adds the new feature test
test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module
docstring to include the required STP link (or, if none, the RFE/Jira epic URL)
for coverage tracking and also populate the empty Jira ticket field in the PR
description; modify the top-of-file docstring in the test module to include a
one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside
the existing summary so automated checks and reviewers can verify the
traceability.
In `@tests/storage/utils.py`:
- Around line 541-584: The helper verify_vtpm_in_windows_vm is labelled "DEBUG
ONLY - MARKED FOR DELETION" yet is referenced by
test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code
marker and make the function a first-class test utility: delete the "# TODO:
Remove - debug only" comment, update the docstring to reflect it is an
intentional test helper (remove "DEBUG ONLY"), and ensure any module
exports/imports remain correct so tests continue to import
verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it,
remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and
delete the function instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11e3ec8d-d6fa-4f0b-9a1f-4e1aa2980145
📒 Files selected for processing (2)
tests/storage/cdi_clone/test_clone.pytests/storage/utils.py
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/wip cancel |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
Clean rebase detected — no code changes compared to previous head ( |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage/cdi_clone/test_clone.py (1)
1-3:⚠️ Potential issue | 🟠 MajorHIGH: PR template section “What this PR does / why we need it” is still not meaningful
The PR description must include substantive content in that section before merge; placeholders/empty text are not acceptable.
As per coding guidelines:##### What this PR does / why we need it:must be present and have meaningful content; missing/placeholder content must be flagged as HIGH severity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cdi_clone/test_clone.py` around lines 1 - 3, PR description currently has a placeholder or missing content for the required section; open the pull request description and populate the heading "##### What this PR does / why we need it:" with a concise, meaningful summary of the change (what the tests in tests/storage/cdi_clone/test_clone.py add or modify, why they are needed, and any important context or impact), ensuring it is not empty or a placeholder before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/storage/cdi_clone/conftest.py`:
- Around line 94-107: The fixture cloned_windows_dv_from_registry_scope_function
declares a parameter source_dv_windows_registry but the actual fixture is named
source_dv_windows_registry_scope_function; update
cloned_windows_dv_from_registry_scope_function to request the correct fixture
name (source_dv_windows_registry_scope_function) or rename the referenced
fixture so names match, ensuring any references inside the function (e.g.,
source_dv_windows_registry.namespace, .size, .name, .storage_class) use the
corrected symbol; verify parametrization now resolves without pytest
fixture-not-found errors.
---
Outside diff comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Around line 1-3: PR description currently has a placeholder or missing content
for the required section; open the pull request description and populate the
heading "##### What this PR does / why we need it:" with a concise, meaningful
summary of the change (what the tests in tests/storage/cdi_clone/test_clone.py
add or modify, why they are needed, and any important context or impact),
ensuring it is not empty or a placeholder before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d5fd69e-5fdd-4828-ad76-25b524b0ae96
📒 Files selected for processing (3)
tests/storage/cdi_clone/conftest.pytests/storage/cdi_clone/test_clone.pyutilities/constants.py
Signed-off-by: Adam Cinko <acinko@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage/cdi_clone/conftest.py (1)
1-1:⚠️ Potential issue | 🟠 MajorHIGH: Restore meaningful content in
What this PR does / why we need itThe PR body currently keeps placeholders without meaningful implementation details. This section is required for reviewer traceability and release auditing.
As per coding guidelines, "
##### What this PR does / why we need it:— MUST be present AND have meaningful content."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cdi_clone/conftest.py` at line 1, The PR body currently contains placeholder text for "##### What this PR does / why we need it:"—replace it with a concise, meaningful summary describing the change (e.g., updates to tests in tests/storage/cdi_clone/conftest.py that add or modify pytest fixtures/imports), why the change was necessary (bugfix, new coverage, refactor), how it was validated (local pytest run, CI), and any backward-compatibility or release notes impact; ensure the section explicitly names the affected module (conftest.py / pytest fixtures) and includes testing instructions so reviewers and auditors can trace the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/storage/cdi_clone/conftest.py`:
- Line 1: The PR body currently contains placeholder text for "##### What this
PR does / why we need it:"—replace it with a concise, meaningful summary
describing the change (e.g., updates to tests in
tests/storage/cdi_clone/conftest.py that add or modify pytest fixtures/imports),
why the change was necessary (bugfix, new coverage, refactor), how it was
validated (local pytest run, CI), and any backward-compatibility or release
notes impact; ensure the section explicitly names the affected module
(conftest.py / pytest fixtures) and includes testing instructions so reviewers
and auditors can trace the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28ed82f7-4e6f-4a37-84d2-5ba8e0b5e760
📒 Files selected for processing (1)
tests/storage/cdi_clone/conftest.py
| client=namespace.client, | ||
| ) as cdv: | ||
| cdv.wait_for_dv_success(timeout=WINDOWS_CLONE_TIMEOUT) | ||
| cdv.wait_for_dv_success(timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
You can use this same way in the timeout in tests/storage/cdi_clone/conftest.py line 108 or either create a constant.py file to address timeout constant and use same in both places.
There was a problem hiding this comment.
got rid of the WINDOWS_CLONE_TIMEOUT constant
There was a problem hiding this comment.
We should always prefer meaningful names instead of magic numbers.
WINDOWS_CLONE_TIMEOUT gives us a clear idea that this number is not random. And in case we need to change it (getting faster/slower env etc.) - it's easier to change it in one place.
There was a problem hiding this comment.
@jpeimer introduced the meaningfully named constant into constants.py and used within the code
| }, | ||
| ) as vm: | ||
| vm.start() | ||
| wait_for_windows_vm(vm=vm, version="2022", timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
Same as before, or you consider cloning timeout should be differentiated from "regular" timeouts? I would say if are same, use TIMEOUT_40MIN as already exists
There was a problem hiding this comment.
done, got rid of the WINDOWS_CLONE_TIMEOUT constant
Signed-off-by: Adam Cinko <acinko@redhat.com>
acinko-rh
left a comment
acinko-rh
left a comment
There was a problem hiding this comment.
applied reviewer's suggestions
| }, | ||
| ) as vm: | ||
| vm.start() | ||
| wait_for_windows_vm(vm=vm, version="2022", timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
done, got rid of the WINDOWS_CLONE_TIMEOUT constant
| client=namespace.client, | ||
| ) as cdv: | ||
| cdv.wait_for_dv_success(timeout=WINDOWS_CLONE_TIMEOUT) | ||
| cdv.wait_for_dv_success(timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
got rid of the WINDOWS_CLONE_TIMEOUT constant
|
Clean rebase detected — no code changes compared to previous head ( |
| with VirtualMachineForTests( | ||
| name=f"vm-{WIN_2K22}-vtpm", | ||
| namespace=namespace.name, | ||
| client=unprivileged_client, | ||
| source="pvc", | ||
| dv_name="dv-target", | ||
| namespace=data_volume_multi_storage_scope_function.namespace, | ||
| size=data_volume_multi_storage_scope_function.size, | ||
| source_pvc=data_volume_multi_storage_scope_function.name, | ||
| storage_class=data_volume_multi_storage_scope_function.storage_class, | ||
| ) as cdv: | ||
| cdv.wait_for_dv_success(timeout=WINDOWS_CLONE_TIMEOUT) | ||
| create_windows_vm_validate_guest_agent_info( | ||
| dv=cdv, | ||
| namespace=namespace, | ||
| unprivileged_client=unprivileged_client, | ||
| vm_params=vm_params, | ||
| ) | ||
| os_flavor=OS_FLAVOR_WIN_CONTAINER_DISK, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(name=U1_LARGE, client=unprivileged_client), | ||
| vm_preference=VirtualMachineClusterPreference(name=WINDOWS_2K22_PREFERENCE, client=unprivileged_client), | ||
| data_volume_template={ | ||
| "metadata": cloned_windows_dv_from_registry_scope_function.res["metadata"], | ||
| "spec": cloned_windows_dv_from_registry_scope_function.res["spec"], | ||
| }, | ||
| ) as vm: |
There was a problem hiding this comment.
When running the test, please look at the DVs in another tab. How many DVs were created by this test? How many did you intend to create?
oc get DV -A -w
with this code, it creates a dv, doing a clone, and then VM creates another clone, but before the change, it was:
- create a dv
- clone a dv
- create a VM that will use the cloned DV (not create a new DV owned by the VM)
There was a problem hiding this comment.
fixed, now it doesn't use volume template thus making a new DV, checked on the cluster itself:
cdi-clone-test-clone dv-source-win2k22-registry 0s
cdi-clone-test-clone dv-source-win2k22-registry 0s
cdi-clone-test-clone dv-source-win2k22-registry PendingPopulation N/A 0s
cdi-clone-test-clone dv-source-win2k22-registry PendingPopulation N/A 1s
cdi-clone-test-clone dv-source-win2k22-registry PendingPopulation N/A 1s
cdi-clone-test-clone dv-source-win2k22-registry ImportScheduled N/A 10s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress N/A 17s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 2.02% 97s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 16.19% 99s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 29.30% 101s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 43.46% 103s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 56.62% 106s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 57.63% 106s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 70.81% 108s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 83.92% 110s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 97.05% 112s
cdi-clone-test-clone dv-source-win2k22-registry ImportInProgress 97.05% 113s
cdi-clone-test-clone dv-source-win2k22-registry Succeeded 100.0% 114s
cdi-clone-test-clone dv-source-win2k22-registry Succeeded 100.0% 114s
cdi-clone-test-clone dv-source-win2k22-registry Succeeded 100.0% 114s
cdi-clone-test-clone dv-target-win2k22-vtpm 0s
cdi-clone-test-clone dv-target-win2k22-vtpm 0s
cdi-clone-test-clone dv-target-win2k22-vtpm PendingPopulation N/A 0s
cdi-clone-test-clone dv-target-win2k22-vtpm PendingPopulation N/A 0s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneScheduled N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneScheduled N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneScheduled N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 1s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 3s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 6s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress N/A 6s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 0.37% 6s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 4.89% 10s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 9.40% 13s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 13.58% 16s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 17.72% 19s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 21.75% 22s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 23.11% 22s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 27.36% 25s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 31.80% 28s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 36.27% 31s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 40.90% 34s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 40.90% 37s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 45.19% 37s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 49.47% 40s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 53.81% 43s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 58.19% 46s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 62.45% 49s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 67.56% 52s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 67.56% 53s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 72.42% 56s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 77.46% 59s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 82.51% 62s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 87.89% 65s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 93.36% 68s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 93.36% 72s
cdi-clone-test-clone dv-target-win2k22-vtpm CloneInProgress 93.36% 72s
cdi-clone-test-clone dv-target-win2k22-vtpm RebindInProgress 100.0% 73s
cdi-clone-test-clone dv-target-win2k22-vtpm RebindInProgress 100.0% 73s
cdi-clone-test-clone dv-target-win2k22-vtpm Succeeded 100.0% 73s
cdi-clone-test-clone dv-target-win2k22-vtpm Succeeded 100.0% 73s
cdi-clone-test-clone dv-target-win2k22-vtpm Succeeded 100.0% 3m32s
cdi-clone-test-clone dv-source-win2k22-registry Succeeded 100.0% 5m28s| client=namespace.client, | ||
| ) as cdv: | ||
| cdv.wait_for_dv_success(timeout=WINDOWS_CLONE_TIMEOUT) | ||
| cdv.wait_for_dv_success(timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
We should always prefer meaningful names instead of magic numbers.
WINDOWS_CLONE_TIMEOUT gives us a clear idea that this number is not random. And in case we need to change it (getting faster/slower env etc.) - it's easier to change it in one place.
| secret=secret, | ||
| cert_configmap=cert.name, | ||
| ) as dv: | ||
| dv.wait_for_dv_success(timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
The import timeout for Win DV can be 50-60 min
There was a problem hiding this comment.
changed to TIMEOUT_60MIN and used as WINDOWS_CLONE_TIMEOUT constant
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3748. Overlapping filesutilities/constants.py |
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4725. Overlapping filesutilities/constants.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4605. Overlapping filestests/storage/cdi_clone/test_clone.py |
Signed-off-by: Adam Cinko <acinko@redhat.com>
| def source_dv_windows_registry_scope_function( | ||
| unprivileged_client, | ||
| namespace, | ||
| storage_class_name_scope_function, | ||
| ): | ||
| """Fixture that creates a Windows 2022 DataVolume from registry.""" | ||
| secret = get_artifactory_secret(namespace=namespace.name) | ||
| cert = get_artifactory_config_map(namespace=namespace.name) | ||
|
|
||
| with create_dv( | ||
| client=unprivileged_client, | ||
| dv_name=f"dv-source-{WIN_2K22}-registry", | ||
| namespace=namespace.name, | ||
| source="registry", | ||
| size=Images.Windows.CONTAINER_DISK_DV_SIZE, | ||
| storage_class=storage_class_name_scope_function, | ||
| url=f"{get_test_artifact_server_url(schema='registry')}/{get_windows_container_disk_path(os_value=WIN_2K22)}", | ||
| secret=secret, | ||
| cert_configmap=cert.name, | ||
| ) as dv: | ||
| dv.wait_for_dv_success(timeout=WINDOWS_CLONE_TIMEOUT) | ||
| yield dv | ||
| cleanup_artifactory_secret_and_config_map(artifactory_secret=secret, artifactory_config_map=cert) |
There was a problem hiding this comment.
the same code is added in #4679
please add in one pr and use when merged
There was a problem hiding this comment.
@rnetser yes, I realised same code is being reused in all of my PRs, I'm working on a PR for test_hotplug.py which adds two new functions into utils.py that manage both VM and DV for windows 2022 from registry, I'm going to used those functions in the other PRs to reduce code duplication
|
/wip |
|
/wip |
|
Clean rebase detected — no code changes compared to previous head ( |
9 participants
Short description:
PR merged to tier-2/3 that makes Windows test VMs in sig-storage use vTPM
(preferably through instancetypes, which should be the right and easy way to achieve this)
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-Authored: Claude Code
https://redhat.atlassian.net/browse/CNV-51351
jira-ticket: