| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-19 | |||
| 14:37:17 | artom | I'm just worried there might be something that implicitly relies on compute service starting up in less than a few seconds | |
| 14:37:33 | dansmith | artom: we have tons of things that are long-running in compute startup, | |
| 14:37:35 | dansmith | so that's not a concern | |
| 14:37:54 | dansmith | we block compute startup until we can talk to conductor in both directions, we clean up failed migrations, do disk IO, etc, etc | |
| 14:38:35 | artom | dansmith, ack, sounds like we have a way forward with moving the warning to _init_host then | |
| 14:42:07 | artom | (dansmith, btw, I suspect it's https://github.com/openstack/nova/blob/master/nova/compute/utils.py#L977 that's the time sink, not https://github.com/openstack/nova/blob/master/nova/compute/utils.py#L975, but that's moot, and unless we ask the reporter we'll never know) | |
| 14:42:44 | dansmith | artom: yeah I just don't know _why_ it'd be slow, unless it's trying to reverse lookup all the addresses too | |
| 14:43:10 | artom | Yeah, good question... | |
| 14:43:24 | openstackgerrit | Eric Fried proposed openstack/nova-specs master: Followup for provider config file spec https://review.opendev.org/671749 | |
| 14:43:35 | artom | If it was doing reverse lookups I think it'd be slower, because a single loopkup takes forever to timeout | |
| 14:43:36 | efried | dansmith: Put the ironic NOTE in a fup so as not to lose gibi's +2 ^ | |
| 14:44:18 | dansmith | artom: that call is implemented in C even | |
| 14:47:31 | alex_xu | stephenfin: sean-k-mooney thanks in person | |
| 14:47:57 | alex_xu | sean-k-mooney: buyin your proposal about 1 extraspec and mask | |
| 14:52:10 | alex_xu | artom: ah...claim depends on migrate_data, just aware that, I need to dig into your patch more | |
| 14:52:40 | artom | alex_xu, hey, if you find a way to make it work, I'm all for it :) | |
| 14:53:28 | alex_xu | at least understand the dependence...and I pretty sure people will hate that I added a interface call get_vpmems :) | |
| 14:55:04 | artom | alex_xu, yeah, that'd make 2 libvirt-specific interfaces that no other driver uses | |
| 14:55:22 | alex_xu | yea | |
| 14:56:51 | artom | Maybe consolidate them into something like "get_driver_specific_migrate_data"? | |
| 14:56:58 | artom | Not super happy with that either | |
| 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 | |