[Storage] [CCLM] Post-CCLM validation tests, Windows VM tests#3926
Open
jpeimer wants to merge 7 commits intoRedHatQE:mainfrom
Open
[Storage] [CCLM] Post-CCLM validation tests, Windows VM tests#3926jpeimer wants to merge 7 commits intoRedHatQE:mainfrom
jpeimer wants to merge 7 commits intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds cross-cluster live-migration (CCLM) tests and fixtures with remote kubeconfig propagation, relocates storage-class constants and helpers to shared modules, moves dv_wait_timeout to parent tests, and extends utilities (artifactory, console, storage) to accept optional client/kubeconfig parameters for remote operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-4
requested review from
Ahmad-Hafe,
RoniKishner,
dalia-frank,
dshchedr,
geetikakay,
kgoldbla,
kshvaika and
rnetser
jpeimer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
utilities/artifactory.py (1)
86-147:⚠️ Potential issue | 🟡 MinorMEDIUM: Document side effects for public utilities that create resources.
Both functions can create/deploy resources; add a Side effects section to the Google‑style docstrings.
Proposed fix
def get_artifactory_secret( @@ Returns: Secret: The Artifactory Secret resource object. Raises: KeyError: If ARTIFACTORY_USER or ARTIFACTORY_TOKEN environment variables are not set. + + Side effects: + Creates and deploys a Secret in the target namespace if it does not exist. """ @@ def get_artifactory_config_map( @@ Returns: ConfigMap: The Artifactory ConfigMap resource object containing the TLS certificate. @@ Raises: KeyError: If server_url is not found in py_config. OSError: If SSL connection to the server fails. + + Side effects: + Creates and deploys a ConfigMap in the target namespace if it does not exist. """As per coding guidelines "Google-format docstrings REQUIRED for all public functions with non-obvious return values OR side effects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/artifactory.py` around lines 86 - 147, Add a "Side effects" section to the Google-style docstrings for get_artifactory_secret and get_artifactory_config_map that clearly states these functions may create and deploy Kubernetes resources (Secret named by ARTIFACTORY_SECRET_NAME via Secret.deploy and ConfigMap "artifactory-configmap" via ConfigMap.deploy), and mention any environment/ config dependencies and possible exceptions (KeyError when ARTIFACTORY_USER/ARTIFACTORY_TOKEN or py_config["server_url"] are missing, and OSError/SSL side effects from ssl.get_server_certificate). Include brief guidance about permissions needed to create resources in the namespace and that existing resources will be returned if present.utilities/console.py (1)
127-133:⚠️ Potential issue | 🟡 MinorMEDIUM: Quote kubeconfig paths to avoid command parsing failures.
If the kubeconfig path contains spaces or shell metacharacters, the virtctl command can misparse. Quoting the path makes command construction robust.
Proposed fix
+from shlex import quote @@ - if self.kubeconfig: - cmd += f" --kubeconfig {self.kubeconfig}" + if self.kubeconfig: + cmd += f" --kubeconfig {quote(self.kubeconfig)}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/console.py` around lines 127 - 133, The constructed command in _generate_cmd uses self.kubeconfig unquoted which can break when the path contains spaces or shell metacharacters; update _generate_cmd to quote the kubeconfig when appending (e.g., use shlex.quote(self.kubeconfig) or wrap in double quotes) so the --kubeconfig argument is always passed as a single token; reference the virtctl_str, self.vm.name, self.vm.namespace and self.kubeconfig variables when making the change.tests/storage/cross_cluster_live_migration/conftest.py (1)
473-544: 🧹 Nitpick | 🔵 TrivialMEDIUM: Unused
remote_cluster_kubeconfigparameter — use@pytest.mark.usefixturesinstead.The
remote_cluster_kubeconfigparameter at lines 477, 499, and 521 is declared as a fixture dependency but never used in the function body (Ruff ARG001). It's there to ensure the kubeconfig file exists before VM creation, which is a dependency-only concern.The idiomatic way to express "I need this fixture's side-effect but not its value" is
@pytest.mark.usefixtures. This removes the Ruff warning and makes the intent explicit:Example for vm_for_cclm_from_template_with_data_source
`@pytest.fixture`(scope="class") +@pytest.mark.usefixtures("remote_cluster_kubeconfig") def vm_for_cclm_from_template_with_data_source( remote_admin_client, remote_cluster_source_test_namespace, remote_cluster_rhel10_data_source, - remote_cluster_kubeconfig, remote_cluster_source_storage_class, ):Apply the same pattern to
vm_for_cclm_with_instance_typeandvm_for_cclm_from_template_with_dv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cross_cluster_live_migration/conftest.py` around lines 473 - 544, The three fixtures vm_for_cclm_from_template_with_data_source, vm_for_cclm_with_instance_type, and vm_for_cclm_from_template_with_dv declare an unused parameter remote_cluster_kubeconfig; remove remote_cluster_kubeconfig from each function signature and instead add `@pytest.mark.usefixtures`("remote_cluster_kubeconfig") directly above each fixture definition so the kubeconfig side-effect runs but the unused-argument warning is eliminated; keep all other parameters and behavior unchanged.tests/storage/storage_migration/utils.py (2)
53-56:⚠️ Potential issue | 🟡 MinorMEDIUM: Assertion message is missing a separator between the two f-strings.
The two f-string literals on lines 54-55 are implicitly concatenated with no space or newline between them. The output will read
"…{'pvc1': 'sc-x'}Doesn't match…"— making the message difficult to parse during test triage. This matters because assertion messages are the primary debugging signal for CI failures.Proposed fix
assert not failed_pvc_storage_check, ( - f"Failed PVC storage class check. PVC storage class: {failed_pvc_storage_check}" - f"Doesn't match expected target storage class: {target_storage_class}" + f"Failed PVC storage class check. PVC storage class: {failed_pvc_storage_check}. " + f"Doesn't match expected target storage class: {target_storage_class}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/storage_migration/utils.py` around lines 53 - 56, The assertion message concatenates two f-strings without a separator causing the PVC dict and the following sentence to run together; update the assertion in tests/storage/storage_migration/utils.py (the line using failed_pvc_storage_check and target_storage_class) to include a clear separator (e.g., add a space or newline between the two f-strings or merge them into a single f-string) so the message reads "...PVC storage class: {failed_pvc_storage_check} Doesn't match expected target storage class: {target_storage_class}".
84-87: 🧹 Nitpick | 🔵 TrivialLOW: Redundant
.strip()onout.Line 86 assigns
out = ...strip(), then line 87 callsout.strip()again. The second.strip()is a no-op.Proposed fix
def verify_file_in_windows_vm(windows_vm: VirtualMachineForTests, file_name_with_path: str, file_content: str) -> None: cmd = shlex.split(f'powershell -command "Get-Content {file_name_with_path}"') out = run_ssh_commands(host=windows_vm.ssh_exec, commands=cmd)[0].strip() - assert out.strip() == file_content, f"'{out}' does not equal '{file_content}'" + assert out == file_content, f"'{out}' does not equal '{file_content}'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/storage_migration/utils.py` around lines 84 - 87, In verify_file_in_windows_vm, remove the redundant second .strip() on out: out is already stripped when assigned (in verify_file_in_windows_vm), so update the assertion to compare out == file_content (or f"'{out}' does not equal '{file_content}'") without calling out.strip() again; this fixes the no-op extra call while keeping the same semantics.tests/storage/cross_cluster_live_migration/test_cclm.py (2)
56-68: 🛠️ Refactor suggestion | 🟠 MajorMEDIUM: Dependency marker lacks a required
# WHYcomment.The
@pytest.mark.dependencyat line 56 and alldepends=references (lines 70, 77, 89, 94, 99) lack the required comment explaining why the dependency exists. The WHY here is straightforward — subsequent tests validate post-migration state and require the migration to have completed successfully — but the guideline is explicit.Example:
# Dependency: post-migration tests require successful CCLM migration to validate state `@pytest.mark.dependency`(name=f"{TESTS_CLASS_NAME_SEVERAL_VMS}::test_migrate_vm_from_remote_to_local_cluster")As per coding guidelines: "When using
@pytest.mark.dependency, a comment explaining WHY the dependency exists is REQUIRED."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cross_cluster_live_migration/test_cclm.py` around lines 56 - 68, Add a brief "# WHY" comment immediately above the `@pytest.mark.dependency` decorator for test_migrate_vm_from_remote_to_local_cluster (and similarly for all depends= references) that explains why the dependency is required (e.g., that subsequent tests validate post-migration state and therefore require the CCLM migration to complete successfully); update the comment text near the pytest.mark.dependency usage so it reads as a one-line rationale tied to the decorator for clarity and compliance with the guideline.
25-33:⚠️ Potential issue | 🟡 MinorAdd
tier3marker to module-level pytestmark—CCLM tests are complex, multi-cluster, and time-consuming.Cross-cluster live migration tests involve multi-cluster coordination, expensive live migration operations, and Windows VM deployments, all of which are textbook characteristics of
tier3tests. The pytest configuration confirmstier3is defined and actively used throughout the codebase for similar complex scenarios. Add the marker at module level to apply it consistently to bothTestCCLMSeveralVMsandTestCCLMWindowsWithVTPMclasses.Proposed fix
pytestmark = [ pytest.mark.cclm, pytest.mark.remote_cluster, + pytest.mark.tier3, pytest.mark.usefixtures(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cross_cluster_live_migration/test_cclm.py` around lines 25 - 33, The module-level pytestmark list is missing the tier3 marker; update the pytestmark definition to include pytest.mark.tier3 so the marker applies to both TestCCLMSeveralVMs and TestCCLMWindowsWithVTPM; locate the pytestmark variable at the top of tests/storage/cross_cluster_live_migration/test_cclm.py and add pytest.mark.tier3 alongside the existing pytest.mark.cclm and pytest.mark.remote_cluster entries.
🤖 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/conftest.py`:
- Around line 592-594: The dv_wait_timeout fixture currently falls back to the
TIMEOUT_30MIN constant and uses a hasattr(request, "param") check; replace the
constant with an explicit numeric value (e.g., 1800) and simplify the logic to
use request.param.get("dv_wait_timeout", 1800) so the default is explicit; also
ensure any callers that pass this fixture value into functions use a named
parameter (e.g., timeout=...) instead of a positional multi-arg call.
In `@tests/storage/cross_cluster_live_migration/conftest.py`:
- Around line 153-171: Replace the single-letter file handle variable used when
writing the kubeconfig with a descriptive name and ensure cleanup remains same;
specifically, in the block that opens kubeconfig_path (currently using "f"),
rename the handle to something like "kubeconfig_file" (or similar) in the with
open(...) as ... statement and update its usage in
yaml.safe_dump(kubeconfig_dict, ...), keeping the existing cleanup try/except
that removes kubeconfig_path and temp_dir and logs via LOGGER.
- Line 148: The kubeconfig file creation writes the remote cluster bearer token
with default file permissions (via open(kubeconfig_path, "w")), making the token
world-readable; change the file creation to use secure permissions (e.g., open
the file descriptor via os.open with mode 0o600 and wrap it with os.fdopen, or
create it then immediately call os.chmod(kubeconfig_path, 0o600)) so
kubeconfig_path is only readable by the owner, and keep the code that writes the
"users": [{"name": user_name, "user": {"token": remote_cluster_auth_token}}]
block unchanged except for using the secured file handle or chmod.
- Around line 558-560: The fixture vms_boot_time_before_cclm currently depends
on vms_for_cclm and yields boot times, creating an implicit ordering dependency;
update its signature to depend on booted_vms_for_cclm instead of vms_for_cclm
and replace the yield with a direct return (no teardown) so it only runs after
VMs are actually booted: change def vms_boot_time_before_cclm(vms_for_cclm,
remote_cluster_kubeconfig) to def vms_boot_time_before_cclm(booted_vms_for_cclm,
remote_cluster_kubeconfig) and return the dict comprehension using
booted_vms_for_cclm.
- Around line 563-569: The fixtures booted_vms_for_cclm and
written_file_to_vms_before_cclm currently use yield but have no teardown; change
them to return the value instead of yielding so they are regular-returning
fixtures (keep the same parameters and scope and keep the running_vm calls and
any pre-setup logic), i.e., remove the generator semantics and ensure the
functions simply perform setup then return vms_for_cclm / the prepared value to
avoid implying a teardown block.
In `@tests/storage/cross_cluster_live_migration/test_cclm.py`:
- Around line 36-52: The parametrize calls use a comma-separated string for
argnames; change them to a tuple of positional argument names: replace the first
argument of pytest.mark.parametrize from "remote_cluster_source_storage_class,
local_cluster_target_storage_class, vms_for_cclm" to a tuple
('remote_cluster_source_storage_class', 'local_cluster_target_storage_class',
'vms_for_cclm') in the TestCCLMSeveralVMs parametrize and make the identical
change for the TestCCLMWindowsWithVTPM parametrize (i.e., update the
pytest.mark.parametrize argnames to a tuple form so pytest receives positional
argnames correctly).
- Around line 136-138: Add a pytest dependency marker to both
test_source_vms_can_be_deleted and test_source_windows_vms_can_be_deleted so
they only run if the migration test in the same test class completed
successfully; specifically, annotate each function with
`@pytest.mark.dependency`(depends=["<migration_test_name>"]) where
<migration_test_name> is the function name of the cross-cluster live migration
test in this class (replace with the actual migration test function name),
ensuring both cleanup tests depend on that migration test.
In `@tests/storage/cross_cluster_live_migration/utils.py`:
- Around line 137-156: Extract the duplicated assertion/reporting logic into a
shared helper like verify_vms_boot_time(local_vms, initial_boot_time,
boot_time_getter) that accepts a boot-time retrieval callable; move the
loop/compare/assert code from verify_vms_boot_time_after_migration and
verify_vms_boot_time_after_storage_migration into that helper, then change
verify_vms_boot_time_after_migration to call it with
get_vm_boot_time_via_console and change
verify_vms_boot_time_after_storage_migration to call it with get_vm_boot_time so
both reuse the same verification and reporting behavior.
In `@tests/storage/utils.py`:
- Around line 29-31: Replace the generic import "from utilities import console"
with an explicit import "from utilities.console import Console" to follow the
specific-import guideline (the file already imports
NO_STORAGE_CLASS_FAILURE_MESSAGE); then update all multi-argument pexpect expect
calls (e.g., vm_console.expect(file_name, timeout=TIMEOUT_20SEC)) to use named
parameters so the pattern is explicit—change them to
vm_console.expect(pattern=file_name, timeout=TIMEOUT_20SEC) (apply this to
instances referencing vm_console, TIMEOUT_20SEC, and file_name).
In `@utilities/storage.py`:
- Around line 627-642: The write_file function lacks type hints, a complete
Google docstring (Returns and Side effects), and should make stop_vm a
keyword-only parameter; update the signature to include type hints for vm
(VirtualMachine), filename (str), content (str), kubeconfig (Optional[str]) and
return type (None) and change stop_vm to be keyword-only (e.g., *, stop_vm: bool
= True); enhance the docstring to add Returns: None and Side effects: may
start/stop the VM and write to disk, and ensure the implementation uses a
try/finally to stop the VM when stop_vm is True (call vm.stop()) after using
console.Console(vm=vm, kubeconfig=kubeconfig) in write_file so the VM is started
if needed and always stopped when requested.
---
Outside diff comments:
In `@tests/storage/cross_cluster_live_migration/conftest.py`:
- Around line 473-544: The three fixtures
vm_for_cclm_from_template_with_data_source, vm_for_cclm_with_instance_type, and
vm_for_cclm_from_template_with_dv declare an unused parameter
remote_cluster_kubeconfig; remove remote_cluster_kubeconfig from each function
signature and instead add `@pytest.mark.usefixtures`("remote_cluster_kubeconfig")
directly above each fixture definition so the kubeconfig side-effect runs but
the unused-argument warning is eliminated; keep all other parameters and
behavior unchanged.
In `@tests/storage/cross_cluster_live_migration/test_cclm.py`:
- Around line 56-68: Add a brief "# WHY" comment immediately above the
`@pytest.mark.dependency` decorator for
test_migrate_vm_from_remote_to_local_cluster (and similarly for all depends=
references) that explains why the dependency is required (e.g., that subsequent
tests validate post-migration state and therefore require the CCLM migration to
complete successfully); update the comment text near the pytest.mark.dependency
usage so it reads as a one-line rationale tied to the decorator for clarity and
compliance with the guideline.
- Around line 25-33: The module-level pytestmark list is missing the tier3
marker; update the pytestmark definition to include pytest.mark.tier3 so the
marker applies to both TestCCLMSeveralVMs and TestCCLMWindowsWithVTPM; locate
the pytestmark variable at the top of
tests/storage/cross_cluster_live_migration/test_cclm.py and add
pytest.mark.tier3 alongside the existing pytest.mark.cclm and
pytest.mark.remote_cluster entries.
In `@tests/storage/storage_migration/utils.py`:
- Around line 53-56: The assertion message concatenates two f-strings without a
separator causing the PVC dict and the following sentence to run together;
update the assertion in tests/storage/storage_migration/utils.py (the line using
failed_pvc_storage_check and target_storage_class) to include a clear separator
(e.g., add a space or newline between the two f-strings or merge them into a
single f-string) so the message reads "...PVC storage class:
{failed_pvc_storage_check} Doesn't match expected target storage class:
{target_storage_class}".
- Around line 84-87: In verify_file_in_windows_vm, remove the redundant second
.strip() on out: out is already stripped when assigned (in
verify_file_in_windows_vm), so update the assertion to compare out ==
file_content (or f"'{out}' does not equal '{file_content}'") without calling
out.strip() again; this fixes the no-op extra call while keeping the same
semantics.
In `@utilities/artifactory.py`:
- Around line 86-147: Add a "Side effects" section to the Google-style
docstrings for get_artifactory_secret and get_artifactory_config_map that
clearly states these functions may create and deploy Kubernetes resources
(Secret named by ARTIFACTORY_SECRET_NAME via Secret.deploy and ConfigMap
"artifactory-configmap" via ConfigMap.deploy), and mention any environment/
config dependencies and possible exceptions (KeyError when
ARTIFACTORY_USER/ARTIFACTORY_TOKEN or py_config["server_url"] are missing, and
OSError/SSL side effects from ssl.get_server_certificate). Include brief
guidance about permissions needed to create resources in the namespace and that
existing resources will be returned if present.
In `@utilities/console.py`:
- Around line 127-133: The constructed command in _generate_cmd uses
self.kubeconfig unquoted which can break when the path contains spaces or shell
metacharacters; update _generate_cmd to quote the kubeconfig when appending
(e.g., use shlex.quote(self.kubeconfig) or wrap in double quotes) so the
--kubeconfig argument is always passed as a single token; reference the
virtctl_str, self.vm.name, self.vm.namespace and self.kubeconfig variables when
making the change.
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-5
removed
the
changes-requested-coderabbitai[bot]
label
jpeimer
changed the title
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cross_cluster_live_migration/conftest.py`:
- Around line 167-172: The cleanup block that calls os.remove(kubeconfig_path)
and os.rmdir(temp_dir) currently catches a bare Exception; change the except
clause to catch OSError instead (e.g. except OSError as e) so only
filesystem-related errors (FileNotFoundError, PermissionError,
IsADirectoryError, etc.) are handled; keep the LOGGER.warning message and
variables kubeconfig_path, temp_dir, and LOGGER unchanged to preserve existing
logging behavior.
In `@tests/storage/utils.py`:
- Around line 530-536: Add INFO-level logging around the VM start and the
console file-check steps: log an INFO message immediately before calling
vm.start(wait=True) to indicate the VM is being started, and another INFO
message after start to indicate the VM is running; likewise, log an INFO message
before entering the console.Console context (or before sending LS_COMMAND)
stating you are checking for file_name with LS_COMMAND and one more INFO after
the expect of file_content to indicate the console file-check completed
successfully. Use the same logger used elsewhere in tests and reference
vm.start, console.Console, LS_COMMAND, file_name, and file_content in the log
messages for clarity.
- Around line 513-536: check_file_in_vm currently has a prohibited defensive
check and lacks INFO logs; remove the "if not vm.ready:" guard and always call
vm.start(wait=True) (vm.start should be idempotent for tests), and add
INFO-level logging immediately before starting the VM and before opening the
console (e.g., logger.info("Starting VM %s", vm) before vm.start(wait=True) and
logger.info("Opening console for VM %s", vm) before with console.Console(...));
reference the check_file_in_vm function, vm.start, and console.Console when
making these edits and create or use a module logger via
logging.getLogger(__name__) if none exists.
In `@utilities/unittests/test_console.py`:
- Around line 103-104: Replace the hardcoded weak password string assigned to
mock_vm.password to avoid Ruff S105; update the assignment in test_console.py
(the mock_vm.password attribute set in the test) from "pass" to a non-sensitive
test value such as "mock-pass" (or "default-pass" to match other tests) so the
static analyzer no longer flags a hardcoded password.
- Around line 101-117: Split the combined
test_console_generate_cmd_with_kubeconfig into two single-purpose test methods:
one that verifies Console(vm=mock_vm, kubeconfig=...) produces the command
including "--kubeconfig" when mock_vm.namespace is set (e.g., name it
test_console_generate_cmd_with_kubeconfig_and_namespace) and a second that
verifies the command when mock_vm.namespace is None (e.g.,
test_console_generate_cmd_with_kubeconfig_without_namespace); keep the same
setup (mock_vm.username/password, patch("console.VIRTCTL", "virtctl")) and
assertions for console.cmd, but move them into the two separate test functions
so each test checks only one behavior of Console._generate_cmd/Console.
---
Duplicate comments:
In `@tests/storage/cross_cluster_live_migration/conftest.py`:
- Around line 574-576: The fixtures vms_boot_time_before_cclm and
written_file_to_vms_before_cclm currently use yield with no teardown code;
change them to return instead (replace yield {...} with return {...}) so they
follow the repository guideline and Ruff PT022 for fixtures without cleanup, and
remove any empty post-yield teardown placeholders; apply the same change to the
duplicate occurrence around the block reported at 579-589.
- Line 668: The helper windows_vm_with_vtpm_for_cclm currently calls vm.start()
a second time (duplicate of earlier starts) causing the double-start issue; edit
the windows_vm_with_vtpm_for_cclm fixture to remove the redundant vm.start()
call (or add a guard such as if not vm.is_running(): vm.start()) so the VM is
only started once, ensuring you reference the vm.start() invocation inside
windows_vm_with_vtpm_for_cclm and adjust any teardown assumptions accordingly.
openshift-virtualization-qe-bot
added
commented-coderabbitai[bot]
tox:verify-tc-requirement-polarion:running
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utilities/unittests/test_console.py`:
- Line 110: The test uses a bare assertion on console.cmd; update the assertion
to include a failure message that shows both expected and actual values so
failures are actionable — change the assertion that references console.cmd (the
existing line asserting "virtctl console test-vm -n test-namespace --kubeconfig
/path/to/kubeconfig") to use the two-argument assert form and include a
descriptive message containing the expected string and console.cmd so test
failures print the observed value.
- Around line 101-111: Add a new unit test that exercises the Console code path
where kubeconfig is provided but namespace is None: create a test (e.g.,
test_console_generate_cmd_with_kubeconfig_no_namespace) that sets
mock_vm.username and mock_vm.password, sets mock_vm.namespace = None, patches
"console.VIRTCTL" to "virtctl", constructs Console(vm=mock_vm,
kubeconfig="/path/to/kubeconfig"), and asserts Console.cmd equals the expected
string without a -n flag (i.e., includes "--kubeconfig /path/to/kubeconfig"
only). This targets the Console._generate_cmd/Console.cmd branch for kubeconfig
+ no namespace.
---
Duplicate comments:
In `@tests/storage/utils.py`:
- Around line 531-533: The defensive check on the schema-provided attribute
vm.ready should be removed or made explicit: either delete the if not vm.ready
branch and assume callers guarantee the VM is running, or add an explicit
parameter (e.g., start_if_not_ready: bool = False) to this helper and gate the
vm.start(wait=True) call on that parameter instead of checking vm.ready;
reference the vm.ready attribute and the vm.start(wait=True) call when making
the change so callers must opt into auto-start behavior.
In `@utilities/unittests/test_console.py`:
- Around line 103-104: The test assigns a real-looking credential to
mock_vm.password which triggers Ruff S105; update the assignment in the test
(the mock_vm.password attribute alongside mock_vm.username) to use a
non-credential placeholder such as "default-pass" (matching other uses in this
file) so the linter no longer flags a hardcoded password.
openshift-virtualization-qe-bot-2
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
rnetser
requested changes
Feb 23, 2026
Collaborator
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
this pr is too big; please split to smaller PRs based on added functionality
| Generate a kubeconfig file from the remote admin client credentials. | ||
| Returns the path to the generated kubeconfig file. | ||
| """ | ||
| # Extract cluster information from the client |
Collaborator
There was a problem hiding this comment.
please use get_client - pass the client config and temp_file_path
| name="dv-fedora-imported-cclm", | ||
| namespace=remote_cluster_source_test_namespace.name, | ||
| source=REGISTRY_STR, | ||
| url=QUAY_FEDORA_CONTAINER_IMAGE, |
|
|
||
|
|
||
| @pytest.fixture(scope="class") | ||
| def vm_for_cclm_from_template_with_dv( |
Collaborator
There was a problem hiding this comment.
iiuc the refered templated here is dataVolumeTemplate and not vm template, if so, the name is confusing
| @pytest.fixture(scope="class") | ||
| def local_vms_after_cclm_migration(admin_client, namespace, vms_for_cclm): | ||
| """ | ||
| Create local VM references for VMs after CCLM migration. |
Collaborator
There was a problem hiding this comment.
the description is confusing - the fixture does not create anything but fetches vms from the cluster
| namespace=remote_cluster_source_test_namespace.name, | ||
| storage_class=remote_cluster_source_storage_class, | ||
| source="http", | ||
| # Using WSL image to avoid the issue of the Windows VM not being able to boot |
|
|
||
| @pytest.mark.dependency(depends=[f"{TESTS_CLASS_NAME_WINDOWS_WITH_VTPM}::test_migrate_windows_vm_with_vtpm"]) | ||
| @pytest.mark.polarion("CNV-14337") | ||
| def test_snapshot_and_restore_windows_vms_after_cclm(self, unprivileged_client, local_vms_after_cclm_migration): |
| raise ValueError(f"Could not determine boot time from output: {output}") | ||
|
|
||
|
|
||
| def get_vm_boot_time_via_console( |
Collaborator
There was a problem hiding this comment.
we already have utilities.virt.get_vm_boot_time -why not use it?
| ) | ||
|
|
||
| @pytest.mark.polarion("CNV-14334") | ||
| def test_source_vms_can_be_deleted(self, vms_for_cclm): |
Collaborator
There was a problem hiding this comment.
the name is misleading - the test verifies that the vms are deleted
| assert not vms_failed_cleanup, f"Failed to clean up source VMs: {vms_failed_cleanup}" | ||
|
|
||
|
|
||
| def delete_file_in_vm( |
Collaborator
There was a problem hiding this comment.
there is already a function to delete file, e.g tests.virt.cluster.vm_lifecycle.test_vm_data_persistency.delete_file
why not use it?
| assert actual_bus == expected_bus, f"Disk {volume.name} has bus '{actual_bus}' but expected '{expected_bus}'" | ||
|
|
||
|
|
||
| def check_file_in_vm( |
Collaborator
There was a problem hiding this comment.
there is already utilities.oadp.check_file_in_running_vm - please re-use
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
7 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.
Short description:
More details:
Jira: https://issues.redhat.com/browse/CNV-60016
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Assisted by Claude AI and Cursor AI
jira-ticket:
Summary by CodeRabbit
Tests
Refactor
Chores