Re: [PATCH v2 1/2] tests/functional/ppc64: extend pseries migration tests

[Aniket Sahu <asahu1x@linux.ibm.com>]

On 19/09/26 12:15 AM, Amit Machhiwal wrote:
> Hi Aniket,
>
> Thanks for the patches.  Please find my comments inline below.
>
> On 2026/09/16 04:06 PM, Aniket Sahu wrote:
>> Made a few changes to test_pseries.py to extend the migration
>> tests to validate exec (socat) and UNIX paths.
>>
>> - Refactored test_ppc64_linux_migration to a private method
>> _do_linux_migration(dst_uri, src_uri=None).
>> - It is then called by three separate methods which exercise the
>> TCP, UNIX and exec paths via the migrate_vms method from
>> MigrationTest.
>> ---
>>   tests/functional/ppc64/test_pseries.py | 41 ++++++++++++++++++++++----
>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/functional/ppc64/test_pseries.py b/tests/functional/ppc64/test_pseries.py
>> index cb6d25dc81..19f31141b1 100755
>> --- a/tests/functional/ppc64/test_pseries.py
>> +++ b/tests/functional/ppc64/test_pseries.py
>> @@ -8,8 +8,9 @@
>>   # later.  See the COPYING file in the top-level directory.
>>   
>>   from qemu_test import Asset
>> -from qemu_test import wait_for_console_pattern
>> +from qemu_test import wait_for_console_pattern, which
>>   from migration import MigrationTest
>> +from qemu_test.ports import Ports
>>   
>>   class PseriesMachine(MigrationTest):
>>   
>> @@ -88,12 +89,20 @@ def test_ppc64_linux_big_boot(self):
>>           wait_for_console_pattern(self, console_pattern, self.panic_message)
>>           wait_for_console_pattern(self, self.good_message, self.panic_message)
>>   
>> -    def test_ppc64_linux_migration(self):
>> +    def _do_linux_migration(self, dst_uri, src_uri=None):
>> +        """
>> +        Boot a pseries kernel on a source VM, wait until Linux is up, then
>> +        migrate to a destination VM over the given URI(s) and verify the boot
>> +        continues on the destination.
>> +        """
>>           self.set_machine('pseries')
>>   
>>           kernel_path = self.ASSET_KERNEL.fetch()
>>           kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE
>>   
>> +        if src_uri is None:
>> +            src_uri = dst_uri
> The default behavior of src_uri=None is non-obvious from the docstring.  Could
> you mention that it defaults to dst_uri, and that callers using transports with
> different source and destination URIs (such as the exec: case below) need to
> provide src_uri explicitly?
>
>> +
>>           dest_vm = self.get_vm(name="dest-qemu")
>>           dest_vm.add_args('-incoming', 'defer')
>>           dest_vm.add_args('-smp', '4')
>> @@ -111,16 +120,38 @@ def test_ppc64_linux_migration(self):
>>           source_vm.set_console()
>>           source_vm.launch()
>>   
>> -        # ensure the boot has reached Linux
>> +        # ensure the boot has reached Linux before migrating
>>           console_pattern = 'smp: Brought up 1 node, 4 CPUs'
>>           wait_for_console_pattern(self, console_pattern, self.panic_message,
>>                                    vm=source_vm)
>>   
>> -        self.migration_with_tcp_localhost_vms(dest_vm, source_vm)
>> +        self.migrate_vms(dst_uri, src_uri, dest_vm, source_vm)
>>   
>> -        # ensure the boot proceeds after migration
>> +        # ensure the boot continues on the destination after migration
>>           wait_for_console_pattern(self, self.good_message, self.panic_message,
>>                                    vm=dest_vm)
>>   
>> +    def test_ppc64_linux_migration(self):
>> +        with Ports() as ports:
>> +            port = ports.find_free_port()
>> +            if port is None:
>> +                self.skipTest('Failed to find a free port')
>> +            self._do_linux_migration('tcp:localhost:%u' % port)
>> +
>> +    def test_ppc64_linux_migration_unix(self):
>> +        self._do_linux_migration(
>> +            'unix:%s/migration.sock' % self.socket_dir().name)
>> +
>> +    def test_ppc64_linux_migration_exec(self):
>> +        if not which('socat'):
>> +            self.skipTest('socat is not available')
>> +        with Ports() as ports:
>> +            port = ports.find_free_port()
>> +            if port is None:
>> +                self.skipTest('Failed to find a free port')
>> +            self._do_linux_migration(
>> +                'exec:socat TCP-LISTEN:%u -' % port,
>> +                'exec:socat - TCP:localhost:%u,forever' % port)
> I share Thomas's concern about CI cost.  With this patch, Linux is fully booted
> three times — once per transport variant — which significantly increases the
> wall-clock time of the pseries migration test slot.
>
> The UNIX and exec transport paths are already covered on ppc64 in
> tests/functional/ppc64/test_migration.py, though against mac99 rather than
> pseries.  If the intent is specifically to validate these transports on the
> pseries machine, that is a reasonable goal — but does it require a full Linux
> boot?  The transport layer is independent of what the guest is running.  A
> firmware-only pseries migration (as was done in v1) would cover the
> pseries-specific migration path with substantially lower CI cost.
>
> Please clarify in the commit message why a full Linux boot is needed here, or
> consider using a firmware-only guest for the two new tests.\

Hi Amit, Thomas,

I do agree that full boot test cases might be unnecessary, however it 
was suggested by Harsh during offline discussions that if a full boot 
test already exists for TCP, so perhaps the best course of action here 
is to revert back to the firmware-only test cases from v1. I do need 
suggestions on if we should extend test_migration.py or make a new file 
for the ppc machines.

Thanks and Regards,
Aniket.