vscsi driver pre_live_migration_on_destination may not be handling hdisk discovery correctly

[Eric Fried]

In nova_powervm.virt.powervm.volume.vscsi.PVVscsiFCVolumeAdapter#pre_live_migration_on_destination, we have the following:

1 # Iterate through host vios list to find valid hdisks.
2 for vios_w in vios_wraps:
3 status, device_name, udid = self._discover_volume_on_vios(
4 vios_w, volume_id)
5 # If we found one, no need to check the others.
6 found = found or hdisk.good_discovery(status, device_name)
7 # if valid udid is returned save in mig_data
8 volume_key = 'vscsi-' + volume_id
9 if udid is not None:
10 mig_data[volume_key] = udid
11
12 if not found or volume_key not in mig_data:
13 ex_args = dict(volume_id=volume_id,
14 instance_name=self.instance.name)
15 raise p_exc.VolumePreMigrationFailed(**ex_args)

- What does the comment on line 5 mean? We're still "checking the others" I think. Should there be a 'break' there? (But then we wouldn't be setting volume_key - see below.)
- We *shouldn't* have empty vios_wraps, but if we did, volume_key would be unset and line 12 would break.
- Line 12 is only checking the *last* volume_key we found. Is that right? Should lines 12-15 be inside the loop?

[Eric Fried (efried)]

Thanks for auto-spacing that for me. Let's try again:

 1 # Iterate through host vios list to find valid hdisks.
 2 for vios_w in vios_wraps:
 3 status, device_name, udid = self._discover_volume_on_vios(
 4 vios_w, volume_id)
 5 # If we found one, no need to check the others.
 6 found = found or hdisk.good_discovery(status, device_name)
 7 # if valid udid is returned save in mig_data
 8 volume_key = 'vscsi-' + volume_id
 9 if udid is not None:
 10 mig_data[volume_key] = udid
 11
 12 if not found or volume_key not in mig_data:
 13 ex_args = dict(volume_id=volume_id,
 14 instance_name=self.instance.name)
 15 raise p_exc.VolumePreMigrationFailed(**ex_args)

[Eric Fried (efried)]

Dammit. I give up. Look at the code.

[Drew Thorstensen (thorst)]

The 'find' (good_discovery) only needs to run on the first VIOS it actually finds something on.

When would/could we have empty VIOSes?

Remember, this is only run for one volume. It would only throw that exception if none of the VIOSes had volumes

[Eric Fried (efried)]

I'm going to assume this is either not really a bug, or not reachable, since nobody else has reported it, and it apparently originated from me staring at code I clearly don't fully understand.