Bug 1513991

Summary: make sure vm_migrate_hook.py works in the domain XML flow
Product: [oVirt] vdsm Reporter: Francesco Romani <fromani>
Component: CoreAssignee: Petr Horáček <phoracek>
Status: CLOSED DEFERRED QA Contact: Raz Tamir <ratamir>
Severity: medium Docs Contact:
Priority: low    
Version: 4.20.4CC: bugs, danken, fromani, michal.skrivanek, ylavi
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-12-27 09:28:18 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: Network RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Exception logged by vm_migrate_hook.py after failed migration none

Description Francesco Romani 2017-11-16 12:52:46 UTC 
Description of problem:
The vm_migrate_hook.py module is using the legacy VM configuration format (vm.conf). It also uses VMFullList to learn about the VM parameters. We need to make sure it works in the new domain XML flow.

Version-Release number of selected component (if applicable):
4.20.6

How reproducible:
100%

Steps to Reproduce:
1. trigger any flow which requires the usage of vm_migrate_hook.py

Actual results:
the module uses the old vm.conf API

Expected results:
the module should use the new Domain XML API
Comment 1 Michal Skrivanek 2017-11-17 06:48:30 UTC 
The whole hook is a horrible hack for ovs. It has tests, do they not pass now?
Either way should be simple to change to use xml
Comment 2 Dan Kenigsberg 2017-11-17 07:53:38 UTC 
Vdsm still reports vm.conf, for backward compatibility with old Engines (and this would continue until we deprecate ovirt-engine-4.1).
But indeed, the hook should move with the times and use domxml when available, so it would not be a reason to keep vm.conf API.
Comment 3 Dan Kenigsberg 2017-11-19 15:08:58 UTC 
Based on bug 1514925 I understand that vm.conf is actually not kept, at least not when the vm is started via xml. Is that correct and intentional? If so, the priority of this bug should be raised.
Comment 4 Francesco Romani 2017-11-20 09:43:01 UTC 
(In reply to Dan Kenigsberg from comment #3)
> Based on bug 1514925 I understand that vm.conf is actually not kept, at
> least not when the vm is started via xml. Is that correct and intentional?
> If so, the priority of this bug should be raised.

Considering Vdsm >= 4.20.z,
you can fully expect vm.conf fully populated and functional if the VM was started with vm.conf (e.g. Engine <= 4.1, or Engine 4.2 with domain xml disabled - still possible AFAIk with hidden setting).
Otherwise, vm.conf is not guaranteed to be fully populated if the VM was started from domain XML. For example, don't expect device data (vm.conf['devices']) to be up to date, or accurate.

We are willing to add data to vm.conf on a per-request basis (bz 1514925 is a good example) to make the transition smoother, but the internal tooling should indeed move on to the new interface ASAP.
Comment 5 Petr Horáček 2017-11-26 16:30:11 UTC 
Created attachment 1359164 [details]
Exception logged by vm_migrate_hook.py after failed migration
Comment 6 Yaniv Lavi 2017-12-27 09:28:18 UTC 
I expect users that use the OVS for host networking to also use OVN.
Comment 7 Michal Skrivanek 2017-12-27 10:07:57 UTC 
great, so can the hook be removed from 4.2?
Besides the current functionality bug, I believe we should eliminate any complex python code from running inside the migration flow (invisible to vdsm, since this is a libvirt hook)
Comment 8 Dan Kenigsberg 2018-04-29 14:58:41 UTC 
The libvirt hook is not yet removed, but since migration *is* working with OVN+physnet fixing it is not a priority.