| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-16 | |||
| 15:37:46 | bauzas | oh wait | |
| 15:37:54 | dansmith | bauzas: you need to purge those too | |
| 15:37:57 | bauzas | dansmith: we don't remove yet also InstanceMapping records? | |
| 15:38:02 | bauzas | a-ha, yeah | |
| 15:38:13 | dansmith | we don't in the case of soft delete, but otherwise we do | |
| 15:38:19 | bauzas | erf | |
| 15:38:30 | bauzas | okay, then I need to look at all the cells, yeah | |
| 15:38:50 | bauzas | I should also purging the instance_mappings too then | |
| 15:38:56 | lyarwood | mdbooth: ah I might be mixing this up with the driver_bdm dict updates that result in a save | |
| 15:39:04 | bauzas | well, okay, I'll see what I can do to use the same command | |
| 15:40:12 | mdbooth | lyarwood: Also, in the previous patch I didn't like the unnecessary argument type ambiguity. But I think I was ok with everything else. | |
| 15:44:04 | lyarwood | mdbooth: the uuid vs bdm_uuid issue? | |
| 15:44:12 | mdbooth | lyarwood: No, just a sec | |
| 15:44:40 | lyarwood | mdbooth: ah the cellv1 values thing | |
| 15:44:54 | mdbooth | https://review.openstack.org/#/c/242602/22/nova/db/sqlalchemy/api.py | |
| 15:45:03 | mdbooth | block_device_mapping_update | |
| 15:45:47 | mdbooth | The caller has full context, and there are multiple ways to pass it in unambiguously | |
| 15:46:32 | lyarwood | mdbooth: right and at the moment that's always the id so we can just drop this for now | |
| 15:47:05 | mdbooth | Well then you'd have to drop the change in update_or_create | |
| 15:47:52 | mdbooth | It shouldn't be a complex fix. | |
| 15:48:09 | lyarwood | ah yeah got it | |
| 15:48:30 | dansmith | bauzas: remind me, we don't delete reqspec at all right now, but will in the case of non-soft delete soon right? | |
| 15:48:47 | dansmith | bauzas: so this command will really only be needed if operators have soft delete enabled, right? | |
| 15:49:02 | bauzas | dansmith: yup, we don't delete the spec yety | |
| 15:49:45 | bauzas | dansmith: if someone hard-deletes an instance (ie. the instance now longer exists), then we should purge the entry | |
| 15:49:52 | mriedem | mdbooth: lyarwood: in case you haven't noticed yet, the original patches to add bdm.uuid were for a cellsv1 race issue where update_or_create happened at the api cell and things got racey and crazy, | |
| 15:50:05 | mriedem | mdbooth: lyarwood: so that's why there is the update_or_create stuff | |
| 15:50:08 | dansmith | bauzas: you're saying you will soon delete reqspec at normal instance delete time, yes? | |
| 15:50:09 | bauzas | dansmith: tbc, I was planning to iterate over all the spec records, and lookup over all cells to see if some instance exists | |
| 15:50:18 | bauzas | for the purge command | |
| 15:50:31 | dansmith | not talking about purge, I'm talking about normal api delete.. | |
| 15:50:35 | mriedem | mdbooth: lyarwood: but i believe that update_or_create is also still called from the compute api (and/or conductor now) when creating the bdms | |
| 15:50:49 | bauzas | dansmith: ah, that, I wasn't planning to introduce it given mriedem's point | |
| 15:50:59 | dansmith | bauzas: his point about what? | |
| 15:51:16 | bauzas | dansmith: the fact that we shouldn't delete the spec record in case this is an instance soft-delete | |
| 15:51:19 | mdbooth | mriedem: IIRC that's correct. The cellsv1 caller was unique in that it passed in a versioned object instead of a list of update values, though. | |
| 15:51:27 | bauzas | dansmith: https://review.openstack.org/#/c/391060/2/nova/compute/api.py@1804 | |
| 15:51:54 | dansmith | bauzas: right but assuming like 1% of people use soft delete, we should do the right thing in the 99% case and only require people that use soft-delete to have to worry about this purge | |
| 15:52:01 | dansmith | I dunno what percentage do use it, but we should always delete if we know we can | |
| 15:52:16 | bauzas | dansmith: so, I could just verify if it's a soft-delete ? | |
| 15:52:42 | bauzas | dansmith: and if it's not a soft-delete, then call the _delete_req_spec method? | |
| 15:52:57 | dansmith | bauzas: yeah, of course, I'm not sure why we wouldn't | |
| 15:53:00 | bauzas | lemme verify, but delete_type seems the good argument for verify it | |
| 15:53:37 | bauzas | dansmith: honestly, purging using the archive command would be super-expensive | |
| 15:54:02 | bauzas | because I need to iterate over all the spec records, and then iterate over all cells for each record, so o(n2) | |
| 15:54:03 | dansmith | bauzas: further, in the purge command, it's going to be very intensive to compare all the current reqspecs with all the deleted=1 instances in a cell, so I think you should just do the api-db purge when you archive an instance row, which will be much more efficient. | |
| 15:54:09 | dansmith | bauzas: yeah, that's completely not okay | |
| 15:54:34 | dansmith | bauzas: doing the purge inside archive_deleted_rows is the only place you can do it efficiently | |
| 15:54:47 | dansmith | a separate purge command will have to do it O(n^2) | |
| 15:55:08 | bauzas | yeah, sorry my keybord doesn't ² :p | |
| 15:55:19 | dansmith | a separate purge command will not be able to effectively determine which deleted instances already have a purged api-db record, | |
| 15:55:21 | dansmith | which makes it expensive | |
| 15:55:26 | dansmith | and after you archive, it's too late | |
| 15:55:37 | bauzas | dansmith: correct me if I'm wrong, but archive_deleted_rows isn't yet cells-aware ? | |
| 15:55:40 | dansmith | but if you do it when you archive, then you can do it in O(n) just like archive is now | |
| 15:56:03 | mdbooth | lyarwood: On locking, I think that everything in api which checks instance/task state should be required to immediately and atomically modify task state. That's probably the level of locking required for rebuild. | |
| 15:56:07 | mriedem | bauzas: the delete_type arg is what tells you the type, | |
| 15:56:07 | dansmith | bauzas: what does it matter if it is? whether you're doing one cell db or all of them, it's the same amount of work | |
| 15:56:15 | mriedem | "soft_delete" or just "delete" | |
| 15:56:22 | mdbooth | I think we do that in many places, btw. | |
| 15:56:41 | bauzas | dansmith: sorry if I'm unclear, but that archive command would have to be run close to a child cell DB, right? | |
| 15:56:54 | sean-k-mooney | jaypipes: looking at https://review.openstack.org/#/c/441183 im not sure that any os-vif change is required | |
| 15:56:54 | bauzas | dansmith: if so, that would be an upcall, right? | |
| 15:57:03 | dansmith | bauzas: it's nova-manage, upcalls are fine | |
| 15:57:29 | sean-k-mooney | jaypipes: the contrial pluging for os-vif would live in the networking-contrail repo not os-vif normally | |
| 15:57:37 | bauzas | dansmith: mmm, okay, I need to consider that | |
| 15:57:42 | mriedem | bauzas: can we just focus on the hard delete case in the api as i pointed out in the review yesterday? and worry about how to best cleanup the soft delete orphans in a separate change? | |
| 15:57:53 | dansmith | yes, please | |
| 15:57:55 | mriedem | they should be separate changes anyway | |
| 15:57:55 | dansmith | not deleting when we know we can is crazypants | |
| 15:57:56 | lyarwood | mdbooth: would that not still allow something like volume-attach to get through? | |
| 15:58:10 | lyarwood | ah, not just me then | |
| 15:58:18 | mdbooth | lyarwood: Not if everything does it, no. | |
| 15:58:23 | bauzas | mriedem: dansmith: yeah, tbc, based on dan's good point, I'm just modifying https://review.openstack.org/#/c/391060/2/nova/compute/api.py to just verify delete_type | |
| 15:58:27 | mdbooth | It's not consistent, though. | |
| 15:58:38 | bauzas | and leave the soft-delete deletions be done differently | |
| 15:58:49 | bauzas | I need to go, ttyl | |
| 15:59:15 | mriedem | bauzas: i said that yesterday in the review :) | |
| 15:59:30 | mriedem | "So we can hard delete the request spec in the API when we hard delete an instance (the delete_type would tell you which it is)." | |
| 15:59:40 | mriedem | "If we care about adding a way to purge leaked request specs (request specs that are related to instances which are deleted), then we could provide a nova-manage command for that, e.g. nova-manage api_db purge_zombies, or something similar. I'd do the purge CLI in a separate patch btw." | |
| 15:59:44 | mdbooth | lyarwood: Anyway, as you say that's not really in scope. | |
| 16:01:56 | mhenkel | sean-k-mooney, Hello | |
| 16:02:05 | sean-k-mooney | jaypipes: that said if the plugin was ovs then yes i would have expected a patch to os-vif | |
| 16:02:07 | sean-k-mooney | mhenkel: hieulq | |
| 16:02:12 | sean-k-mooney | mhenkel: *hi | |
| 16:02:32 | sean-k-mooney | mhenkel: just dissusing your change | |
| 16:02:59 | sean-k-mooney | mhenkel: do im not sure jay is about currently | |
| 16:03:05 | mhenkel | I was just debating internally if we should upstream https://github.com/Juniper/contrail-nova-vif-driver/tree/master/vif_plug_vrouter to os-vif | |
| 16:03:35 | mhenkel | or keep it as a separate plugin delivered by contrail packages | |
| 16:04:33 | sean-k-mooney | mhenkel: currently the policy for plugins in os-vif is that they can only be fro the reference backends | |
| 16:04:37 | mhenkel | my personal preference would be to upstream | |
| 16:04:45 | sean-k-mooney | so ovs,linux bridge and sriov | |
| 16:04:49 | mhenkel | ah ok | |
| 16:05:23 | sean-k-mooney | that is something we could change but the resoning is that it allows you to maintain and update them faster yourself | |
| 16:05:42 | mhenkel | sean-k-mooney: yes, that I agree with | |
| 16:06:29 | sean-k-mooney | mhenkel: one thing that will be changing this cycle hopefully is that neutron will start passing os-vif vif object to nova | |
| 16:07:19 | mhenkel | sean-k-mooney: so we should be good with what I upstreamed so far? | |
| 16:07:23 | sean-k-mooney | mhenkel: when that happens the change you are proposing will nolonger be required as you will be able to spcify the plug to use for the ml2 mech dirver | |
| 16:07:31 | sean-k-mooney | mhenkel: yes | |
| 16:07:46 | sean-k-mooney | mhenkel: though one question | |