migration: Fix permission denied issue in shared filesystems

[cliping]

The permission denied issue was caused by an incorrect mount order. The local path needs to be mounted first, and then mount the path on the target host.

Before:
VM 'avocado-vt-vm1' failed to start: error: Failed to start domain 'avocado-vt-vm1' error: internal error: process exited while connecting to monitor: 2025-12-19T10:33:35.319104Z qemu-kvm: -blockdev {"driver":"file","filename":"/var/lib/libvirt/images/nfs-link/jeos-27-x86_64.qcow2","aio":"native","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap","cache":{"direct":true,"no-flush":false}}: Could not open '/var/lib/libvirt/images/nfs-link/jeos-27-x86_64.qcow2': Permission denied(exit status: 1)

After:
(1/1) type_specific.io-github-autotest-libvirt.migration.migration_misc.non_canonical_paths_in_shared_filesystems: STARTED
(1/1) type_specific.io-github-autotest-libvirt.migration.migration_misc.non_canonical_paths_in_shared_filesystems: PASS (180.13 s)
RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /var/log/avocado/job-results/job-2026-01-04T03.36-e9cba29/results.html
JOB TIME : 181.91 s

Summary by CodeRabbit

• Tests
  • Updated test configuration for shared filesystem migration scenarios
  • Enhanced NFS mount handling with improved session management and verbose logging capabilities
  • Refined test artifact cleanup processes to ensure proper resource removal

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This pull request modifies the shared filesystem test infrastructure for libvirt migration testing. A configuration file updates the base export directory path from "/nfs" to "/var/shared_filesystems", which propagates to all derived filesystem paths. The corresponding test Python file is refactored to perform NFS mounts over an established SSH session with verbose logging, replaces the symlink creation mechanism with a local process-based approach, and extends the cleanup procedure to remove the newly created local artifacts.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: addressing a permission denied issue in shared filesystems during migration. It aligns with the PR's core objective of fixing mount order problems that caused qemu permission errors.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

libvirt/tests/src/migration/migration_misc/non_canonical_paths_in_shared_filesystems.py (3)

54-58: Correct mount order! Consider adding verbose logging to local mounts for consistency.

The mount order correctly addresses the PR objective: local paths are mounted first (lines 50-52), then remote paths are mounted (lines 54-58). This prevents the "Permission denied" issue.

However, for consistency, the local mounts (lines 50-52) should also use verbose=True for better debugging visibility.

🔎 Proposed refactor for consistent verbose logging

-    utils_disk.mount(nfs_images_path, images_path, options=nfs_mount_options)
-    utils_disk.mount(nfs_nvram_path, nvram_path, options=nfs_mount_options)
-    utils_disk.mount(nfs_swtpm_path, swtpm_path, options=nfs_mount_options)
+    utils_disk.mount(nfs_images_path, images_path, options=nfs_mount_options, verbose=True)
+    utils_disk.mount(nfs_nvram_path, nvram_path, options=nfs_mount_options, verbose=True)
+    utils_disk.mount(nfs_swtpm_path, swtpm_path, options=nfs_mount_options, verbose=True)

---

60-61: Consider using && for better error handling.

The symlink creation command chains two operations with ;, which means the symlink creation will proceed even if the directory creation fails. Using && would be more defensive.

🔎 Proposed refactor

-    cmd = f"mkdir {images_path}/nfs; ln -s {images_path}/nfs {images_path}/nfs-link"
+    cmd = f"mkdir {images_path}/nfs && ln -s {images_path}/nfs {images_path}/nfs-link"

---

104-104: Consider more specific cleanup pattern.

The wildcard pattern nfs* will remove any file or directory starting with "nfs" in the images_path. For more precise cleanup, explicitly remove only the artifacts created during setup.

🔎 Proposed refactor for more precise cleanup

-    process.run(f"rm -rf {images_path}/nfs*", shell=True, ignore_status=True)
+    process.run(f"rm -rf {images_path}/nfs-link {images_path}/nfs", shell=True, ignore_status=True)

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91d7aee and caae54f.

📒 Files selected for processing (2)

• libvirt/tests/cfg/migration/migration_misc/non_canonical_paths_in_shared_filesystems.cfg
• libvirt/tests/src/migration/migration_misc/non_canonical_paths_in_shared_filesystems.py

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-12-12T10:00:09.383Z

Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.

Applied to files:

• libvirt/tests/cfg/migration/migration_misc/non_canonical_paths_in_shared_filesystems.cfg

📚 Learning: 2025-09-24T08:01:27.899Z

Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

• libvirt/tests/src/migration/migration_misc/non_canonical_paths_in_shared_filesystems.py

🧬 Code graph analysis (1)

libvirt/tests/src/migration/migration_misc/non_canonical_paths_in_shared_filesystems.py (2)

libvirt/tests/src/serial/serial_functional.py (1)

• wait_for_login (235-258)

provider/vfio/ccw.py (1)

• mount (137-148)

🪛 Ruff (0.14.10)

libvirt/tests/src/migration/migration_misc/non_canonical_paths_in_shared_filesystems.py

61-61: Function call with shell=True parameter identified, security issue

(S604)

---

104-104: Function call with shell=True parameter identified, security issue

(S604)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Python 3.11
• GitHub Check: Python 3.12
• GitHub Check: Python 3.8
• GitHub Check: Python 3.9

🔇 Additional comments (1)

libvirt/tests/cfg/migration/migration_misc/non_canonical_paths_in_shared_filesystems.cfg (1)

33-33: LGTM! More descriptive export directory path.

The change from /nfs to /var/shared_filesystems is more descriptive and clearly indicates the purpose. The variable substitution will correctly propagate this change to all dependent paths.