| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-26 | |||
| 14:26:04 | sean-k-mooney | yes the second one | |
| 14:26:07 | efried | (which I thought was today, but I guess not) | |
| 14:26:22 | sean-k-mooney | i think it is still nessacary for some feature but we have a backlog of feature to catch up on | |
| 14:26:40 | openstackgerrit | Merged openstack/nova master: Document archive_deleted_rows return codes https://review.opendev.org/677819 | |
| 14:27:03 | efried | sean-k-mooney: I actually don't remember the last time we did something in nova that *needed* to track against unreleased placement. | |
| 14:27:35 | sean-k-mooney | well all of the nested stuff would have needed it if we did that this cycle. | |
| 14:27:35 | efried | and iiuc having it set up this way allows us to do things like... merge a nova feature before placement is released accordingly, thereby breaking nova (cf. the aforementioned fixture issue). | |
| 14:27:43 | efried | yeah, but we can just release placement | |
| 14:27:50 | efried | releases are pretty cheap. | |
| 14:28:18 | sean-k-mooney | maybe. i guess depends on is really the way to test this stuff | |
| 14:29:15 | efried | right, I'm saying, I'm not sure there's a good reason for us to have placement in required projects by default. | |
| 14:29:40 | sean-k-mooney | i wasnt refering to required proejcts specifcally | |
| 14:30:05 | efried | that's how we end up with nova running against master placement in the gate, nah? | |
| 14:30:40 | mriedem | you need placement in required projects in the gate jobs b/c it's a service not a library | |
| 14:30:55 | mriedem | if you want to freeze the fixture or something, yo'ud need to make it a library i think | |
| 14:31:17 | openstackgerrit | Merged openstack/nova master: Change nova-manage unexpected error return code to 255 https://review.opendev.org/677832 | |
| 14:31:20 | efried | do we have keystone/glance/ironic/neutron/cinder in required projects too? | |
| 14:31:21 | mriedem | iow, if placement isn't in required-projects, gate jobs aren't going to run with placement from master | |
| 14:31:24 | mriedem | yes | |
| 14:31:25 | openstackgerrit | Merged openstack/nova master: Document map_instances return codes in table format https://review.opendev.org/677835 | |
| 14:31:50 | efried | okay, maybe I'm thinking of placement as more of a lib | |
| 14:32:34 | dansmith | artom: you working on the test failures? I haven't looked to see what they are, but I assume they're relevant since all of them seem to be failing | |
| 14:32:40 | mriedem | https://github.com/openstack/devstack/blob/master/.zuul.yaml#L368 | |
| 14:32:44 | artom | dansmith, yeo | |
| 14:32:46 | artom | *yep | |
| 14:33:03 | artom | dansmith, wanted to ask you, we can't fully remove an RPC param until the next major version bump, right? | |
| 14:33:38 | artom | I wanted to remove destroy_disks from the rollback_live_migration call and have the method on the destination do all the deciding, since _live_migration_flags isn't host-dependant, it only cares about migrate_data. | |
| 14:33:53 | artom | But we have to keep compatibility code until RPC 6.0, right? | |
| 14:34:21 | dansmith | artom: fully or otherwise, yeah | |
| 14:35:30 | artom | dansmith, wait, otherwise? So not at all? RPC params are only additive? | |
| 14:36:13 | dansmith | artom: yeah, meaning if you supported the letter and intent of the law in the current version, you can't stop honoring either until the next bump | |
| 14:36:16 | mriedem | what does destroy_disks have to do with numa? | |
| 14:36:31 | dansmith | artom: in that, you can't just accept and ignore some flag that has some meaning | |
| 14:37:19 | artom | mriedem, it's convoluted, but basically, we need to call rollback_live_migration_at_destination if we did a claim and we need to drop it | |
| 14:37:35 | artom | Except now, it's called only the do_cleanup is True (as determined by _live_migration_flags) | |
| 14:38:08 | mriedem | yeah i remmeber that | |
| 14:38:22 | artom | There's a bunch of possibilities around that, but the end result it, everything becomes easier if we just always call rollback_live_migration_at_destination, and let it decide what cleanup needs to be done | |
| 14:38:23 | mriedem | you mean rollback_live_migration_at_destination right? | |
| 14:38:36 | artom | Instead of passing it booleans over RPC | |
| 14:38:53 | artom | Except doing that would technically require removing the destroy_disks param | |
| 14:39:47 | dansmith | artom: don't we pass that flag because there are cases where the other side can't decide what to do? | |
| 14:41:02 | artom | dansmith, might have been the case at one time, but doesn't look like it now. https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6946-L6979 doesn't depend on anything but migrate_data, which both sides have | |
| 14:41:11 | artom | Unless I'm really blind | |
| 14:41:26 | artom | I guess it was done to save an RPC call if we didn't need it? | |
| 14:43:55 | artom | sean-k-mooney, could you weigh in please: https://review.opendev.org/#/c/635669/39/nova/compute/resource_tracker.py@307 | |
| 14:44:10 | dansmith | hmm, okay.. I really thought we had serious issues with that logic, | |
| 14:44:23 | dansmith | but perhaps the passing of migrate data to more places made that better or something | |
| 14:44:25 | dansmith | it's been a while | |
| 14:44:34 | dansmith | mriedem probably remembers better | |
| 14:44:52 | mriedem | i would have to dig, _live_migration_cleanup_flags sucks | |
| 14:44:57 | dansmith | yeah | |
| 14:45:40 | mriedem | i had to workaround the same kind of thing where do_cleanup was False but i still needed to do network_api.setup_networks_on_host for the dest in the neutron case with port bindings | |
| 14:46:01 | mriedem | https://review.opendev.org/#/q/I658e0a749e842163ed74f82c975bcaf19f9f7f07 | |
| 14:47:24 | mriedem | artom: so you just need the source to make a call to the dest to rollback the claim on failure right? regardless of shared storage or not | |
| 14:47:32 | artom | mriedem, yep | |
| 14:47:45 | artom | I could always just add a new RPC method | |
| 14:47:48 | mriedem | dansmith probably won't like what i'm about to suggest, but it might be better to just write a new rpc method, | |
| 14:47:56 | artom | Seems weird to do that since we already have a thing called "rollback" | |
| 14:47:57 | mriedem | rather than munge it into this steaming pile o shit | |
| 14:48:08 | artom | You don't like wobbly shit castles? | |
| 14:48:26 | artom | If dansmith's onboard I'm all for that | |
| 14:48:48 | mriedem | we'd only make this new call if we know, from the source, that there would be something to cleanup, right? meaning the instance has numa claims in the migrate_data object or something? | |
| 14:48:49 | dansmith | mriedem: I dunno why you'd say that... making another call that does what we want instead of calling this one and expecting very specific behavior is better, IMHO | |
| 14:49:06 | mriedem | dansmith: b/c of our prep_resize hullabaloo a couple of weeks back | |
| 14:49:35 | artom | 'tis settled. | |
| 14:49:56 | dansmith | mriedem: yeah, I mean, okay fair point, but that seemed a little more knife edge single-case to me | |
| 15:03:01 | sean-k-mooney | artom: oh hi yes | |
| 15:03:03 | mriedem | if i re-use the existing prep_resize, i will either have to pass it a new param to control the logic or change it to check if the migration.cross_cell_move flag is set and True (which won't work for old computes, but that could be filtered out in the api or conductor based on compute service version), but either way i'd need to change it to say (1) don't reschedule and (2) don't rpc cast to resize_instance on the source | |
| 15:03:16 | sean-k-mooney | had neutron open for some reason ill take a look | |
| 15:09:20 | dansmith | artom: the new call would be "unclaim $this on the dest" yeah? | |
| 15:09:45 | sean-k-mooney | we used the migration_data for a few reason on of which was move claims were only used for cold migration and we did not want to have to depend on the for sriov migration. | |
| 15:09:55 | artom | dansmith, yep | |
| 15:10:11 | sean-k-mooney | we already had the old vif bindings for the souce and the new vif bindigns for the dest in the migration data | |
| 15:10:31 | dansmith | artom: and what if the source rebooted because of the failure and never calls that? | |
| 15:10:39 | sean-k-mooney | so it was simple to extend that to also have the pci info | |
| 15:10:48 | dansmith | artom: I don't think I was clear about where you're persisting anything from the claim you're doing, other than in memory | |
| 15:11:05 | sean-k-mooney | the only thn we neede was the pci adress which is stored in the vifs port profile | |
| 15:11:24 | artom | dansmith, claims aren't persisted anywhere. The instance has a migration context though | |
| 15:11:46 | artom | dansmith, the update resources periodic would clean up if we don't drop the claim "manually", I believe | |
| 15:12:10 | dansmith | artom: right, the claims themselves aren't, but | |
| 15:12:18 | dr_gogeta86 | hi | |
| 15:12:22 | dansmith | you mean the periodic would find that the migration wasn't associated with us anymore and clean up the in-memory state on the dest | |
| 15:12:57 | artom | dansmith, wait, what do you mean by in-memory state? | |
| 15:13:16 | artom | dansmith, it would update the available resources in the database... | |
| 15:13:38 | dansmith | artom: that's my point, I'm not sure where we're accouting for these resources in the database | |
| 15:13:51 | sean-k-mooney | artom: in the host_cell object in the numa toplogy blob in the compute node | |
| 15:14:03 | artom | dansmith, ^^ there you go :) | |
| 15:14:24 | dansmith | where are we updating that? | |
| 15:14:56 | artom | dansmith, lemme dig | |
| 15:15:03 | dansmith | because I don't see it | |
| 15:15:05 | artom | Knowing sean-k-mooney he might pull it out in seconds | |
| 15:15:29 | sean-k-mooney | dansmith: when we claim the cpus and hugepages in pre live migereation at dest we shoudl update the db | |
| 15:15:50 | sean-k-mooney | that is where we claim the pci deivces for sriov live migration | |
| 15:15:52 | dansmith | sean-k-mooney: yeah, stop using words like "should" and handwaving with "the database".. because that doesn't help :) | |
| 15:16:03 | dansmith | sure, pci devices are claimed with the manager, I get tat | |
| 15:16:31 | dansmith | I don't think we ever claimed the basic resources in the db before we removed all that stuff | |
| 15:16:38 | dansmith | so I'm not sure where this is happening | |
| 15:16:51 | dansmith | mriedem: do you know? | |
| 15:17:07 | artom | dansmith, we don't care about the basic resources anymore, do we? | |
| 15:17:11 | artom | They're all in placement | |