Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-19
14:57:49 efried fwiw, I think we need *something* like this ^
14:58:08 efried although... I thought we already *had* driver-specific migrate data
14:58:20 artom Exactly
14:58:29 artom It's more like... claims-dependant driver-specific
14:58:32 artom Which is a mouthful
14:59:27 artom Or... post-claim migrate_data
14:59:30 artom Would that work?
15:00:04 openstackgerrit Stephen Finucane proposed openstack/nova master: Use a less chipper title for release notes https://review.opendev.org/671752
15:02:01 artom driver.migrate_data_from_claim(context, instance, claim)
15:02:11 artom efried, alex_xu ^^ how does that sound?
15:03:47 efried artom: What are these LiveMigrateData subclasses for, if not for this?
15:04:01 dansmith I'm not following this,
15:04:09 dansmith but we do have per-driver migrate data for exactly this reason
15:04:11 artom efried, it's a sequencing problem
15:04:15 dansmith and that's what those subclasses are
15:04:18 artom See https://review.opendev.org/#/c/634606/44/nova/compute/manager.py@6482
15:04:50 artom It's not a migrate_data class problem, it's "we need to call the driver twice, once for check_can_live_migrate_destination, and once after we've done claims"
15:05:04 artom In my patch, that second call is very use-case specific "get_dst_numa_config"
15:05:36 sean-k-mooney for what its woth we do something similar for sriov
15:05:39 artom alex_xu will need to solve the same problem, so rather than adding "get_vpmem_config" or whatever to the driver interface, we're trying to come up with a more generic single method for that second call to driver
15:06:22 alex_xu I'm trying to figure out why you need the migration_data for the claim
15:06:40 artom Other way around, we need to claim to update the migrate data
15:06:51 artom Because we get the NUMA topology on the dest host from the claim
15:09:42 alex_xu artom: looks like you only want to know whether instance has numa topo https://review.opendev.org/#/c/634606/44/nova/compute/manager.py@6470
15:10:13 sean-k-mooney for sriov migration we added https://review.opendev.org/#/c/620115/35/nova/compute/resource_tracker.py which we callin in https://review.opendev.org/#/c/620115/35/nova/conductor/tasks/live_migrate.py
15:10:28 sean-k-mooney * in check_can_live_migrate_destination
15:11:34 artom alex_xu, in my case, yeah
15:12:29 alex_xu artom: I don't understand this https://review.opendev.org/#/c/634606/44/nova/virt/libvirt/driver.py@7463
15:13:00 artom alex_xu, it's to handle the N/N-1 problem
15:13:04 alex_xu what different with we check instance.numa_topology at dest host compute manager
15:13:13 alex_xu oh, upgrade issue
15:13:18 artom Exactly
15:13:35 sean-k-mooney alex_xu old host wont populate that field
15:13:38 artom But you're right, once we get to U or V or whenever, that's no longer needed
15:13:55 openstackgerrit **** proposed openstack/nova master: Nova: node should be deleted when last service is deleted https://review.opendev.org/671731
15:13:58 sean-k-mooney well actuly ignore that
15:14:46 sean-k-mooney they wont populate it but we need it for other reasons
15:14:47 alex_xu do we need that LM to be fixed in the middle of S to T upgrade
15:15:05 alex_xu if not, then we can just enable the numa LM by the rpc version
15:15:10 artom Are you asking whether we need to support S -> T NUMA LM?
15:15:31 alex_xu we can only support that LM when all the nodes upgrade to T
15:15:35 sean-k-mooney alex_xu: we need to assume that not all nodes will be upgrade at teh same time
15:15:43 artom We do, it's apparently a big thing during upgrades, operators migrate workloads off compute nodes being upgraded
15:16:02 sean-k-mooney alex_xu: that was discussed at lenght in the spec
15:16:28 artom Or if you're asking "why not check min service version" to determine whether we can NUMA LM...
15:16:37 sean-k-mooney alex_xu: https://specs.openstack.org/openstack/nova-specs/specs/train/approved/numa-aware-live-migration.html#upgrade-impact
15:16:48 artom Yeah, we could do that, and at point it's what was done in the code, but I think dansmith came up with this way, which personally I find more elegant
15:17:44 alex_xu the min service version is also require all the nodes upgrade to T, right?
15:17:50 artom Yeah
15:18:30 artom Although perhaps, in the interest of not adding driver interface methods, we could go back to checking min service version, and then we could do the claim before driver.check_can_live_migrate_at_destinaton
15:18:33 artom (I think)
15:18:52 sean-k-mooney we should not do the claim before that
15:19:07 sean-k-mooney driver.check_can_live_migrate_at_destinaton is where the claim should be done
15:19:29 alex_xu that check means, only the source node is new version, then we can do LM?
15:19:53 sean-k-mooney alex_xu: we can live migrate but we wont update the guest xml
15:20:24 sean-k-mooney so it will be jsut as broken as before
15:20:38 sean-k-mooney but it will live migrate succesfully
15:21:06 alex_xu sean-k-mooney: so in upgrade, we need to LM from the old to new, then LM from new back to old
15:21:24 sean-k-mooney you dont need too
15:21:30 sean-k-mooney we just wont break that
15:22:00 sean-k-mooney the sriov migration feature is coded such that it falls back to the old behavior if the info is not available to use teh new flow
15:22:28 sean-k-mooney the new flow is basically only used when both are new enough but its explained in the table in the spec
15:23:32 alex_xu but I still can't migrate the instance from old to new...that break the first step of upgrade
15:23:42 sean-k-mooney you can
15:23:55 alex_xu you said the xml won't update
15:24:00 sean-k-mooney correct
15:24:11 sean-k-mooney it use the old live migration workflow
15:24:13 artom It's impossible to update the XML unless both source and dest support it
15:24:24 artom So we do best effort and revert to previous half-broken behaviour
15:24:48 alex_xu the instance won't be break?
15:24:54 artom It might :)
15:25:08 sean-k-mooney they havent been broken for the last 10 releases
15:25:08 artom That's why it's currently half-broken
15:25:09 alex_xu hah
15:25:25 sean-k-mooney what might happen is two vms get pinned to the same cores
15:25:36 sean-k-mooney but that could happen before.
15:25:51 artom It might actually break if it's pinned to CPUs that don't exist on the dest
15:26:14 alex_xu yea, so...we should ask the user to do that
15:26:14 sean-k-mooney if the cores do not exist or there is not enough hugepages libvirt fails the migrtion due to the qemu error
15:26:17 sean-k-mooney and it auto reverts
15:26:25 alex_xu s/should/shouldn't/
15:26:28 sean-k-mooney that is the same behavior as today
15:27:10 alex_xu I remember we have a patch stop that LM api call by checking the instance has numa
15:27:19 sean-k-mooney yep we merged that
15:27:25 artom Yeah, it's now disabled by default
15:28:03 sean-k-mooney so to live migrarte form S->T they need to re enabeld it
15:28:21 sean-k-mooney although i think artom is removing that option in T?
15:28:27 sean-k-mooney or is that puhsed to U
15:28:31 alex_xu so...we will tell the user, now you can LM your instance again when you upgrade from S to T, but you instance may gone based on lucky :)
15:28:41 artom sean-k-mooney, it's being deprecated but not removed, because of the upgrade impact
15:28:56 artom We don't want to turn it on by default unless we're sure the whole cloud supports it
15:29:33 sean-k-mooney alex_xu: the important thing is we do not regres or change any live migration behavior with artom code
15:30:01 alex_xu sean-k-mooney: I agree with that...but doesn't mean we should encourge the user to do that
15:30:14 alex_xu special for danger thing
15:30:22 sean-k-mooney we are not encuragin people to do it
15:30:38 sean-k-mooney if you have numa instance you should cold migrate them or do an inplace upgrade
15:30:50 sean-k-mooney that is what we will be telling customers
15:31:11 alex_xu yes, so we needn't to support src to dest numa instance LM :)
15:31:18 alex_xu for s to t
15:31:41 sean-k-mooney alex_xu: orginally we didnt

Earlier   Later