| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-26 | |||
| 14:23:15 | sean-k-mooney | yep seams like what we should be doing | |
| 14:23:20 | mordred | the sdk is going to give you the happy fixture | |
| 14:23:22 | mordred | and you will use that | |
| 14:23:25 | mordred | and it will be tested in the sdk | |
| 14:23:29 | mordred | so we won't break you | |
| 14:23:36 | efried | right, and then as they change internals, they'll change the fixture, and we'll automatically "keep up". | |
| 14:23:48 | mriedem | i might as well just fast approve this | |
| 14:23:55 | mriedem | it's test only and holding up the u-c change | |
| 14:24:05 | efried | yes, the u-c patch proves it works | |
| 14:24:12 | sean-k-mooney | efried: we try to do that with plamcent but we have been broken with the placement fixture in the past | |
| 14:24:12 | efried | and the nova patch proves it still works at 0.34.0 | |
| 14:24:24 | efried | sure, but less likely | |
| 14:24:39 | sean-k-mooney | we will need to similarly pull the sdk out of requirements and install it spereatly in tox right | |
| 14:24:48 | sean-k-mooney | e.g. to consume it form master? | |
| 14:24:51 | efried | eh? Why would we need to do that? | |
| 14:25:02 | sean-k-mooney | the same reason we do it for the placement fixture | |
| 14:25:13 | efried | oh, no, tracking against real releases should be fine. | |
| 14:25:22 | sean-k-mooney | ok | |
| 14:25:34 | efried | When we've needed to pre-test a feature, we twiddle the project list in .zuul.yaml | |
| 14:25:34 | sean-k-mooney | i guess for placement we do it to get eraly acess to features | |
| 14:25:50 | efried | yes, which is actually probably not necessary anymore | |
| 14:25:55 | efried | actually probably not advisable. | |
| 14:26:01 | efried | we should bring that up with cdent when he gets back. | |
| 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 | 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:35 | sean-k-mooney | well all of the nested stuff would have needed it if we did that this cycle. | |
| 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 | |