Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-16
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
16:08:00 sean-k-mooney for contrial are you managin ovs or do you support other backends
16:08:35 mhenkel no ovs, we provide our own virtual router
16:08:39 openstackgerrit Dan Smith proposed openstack/nova master: Get instance availability_zone without hitting the api db https://review.openstack.org/439754
16:08:40 openstackgerrit Dan Smith proposed openstack/nova master: Avoid lazy-loading projects during flavor notification https://review.openstack.org/445697
16:08:43 openstackgerrit Dan Smith proposed openstack/nova master: Set instance.availability_zone whenever we schedule https://review.openstack.org/446053
16:08:45 openstackgerrit Dan Smith proposed openstack/nova master: Make conductor ask scheduler to limit migrates to same cell https://review.openstack.org/438025
16:08:51 mhenkel either as a kmod or in userspace using dpdk
16:09:03 sean-k-mooney mhenkel: ah ok. in that case what you have is perfect
16:09:34 mhenkel sean-k-mooney: so we just need to convince jaypipes to take back his -1 ;)
16:09:37 sean-k-mooney mhenkel: if you were managing ovs(with or without dpdk) you could upstream the changes that are required to the ovs plugin
16:09:45 mhenkel I see
16:10:26 sean-k-mooney mhenkel: yes but jaypipes will likely agree when he reads my comment
16:11:15 openstackgerrit Sujitha proposed openstack/nova master: Add helper method to add additional data about policy rule. https://review.openstack.org/434842
16:12:39 melwitt mriedem: on the regression test, do you know why you didn't need RetryFilter?
16:13:40 openstackgerrit Sujitha proposed openstack/nova master: Add helper method to add additional data about policy rule. https://review.openstack.org/434842
16:15:45 openstackgerrit Stephen Finucane proposed openstack/os-vif master: Use Sphinx 1.5 warning-is-error https://review.openstack.org/446616

Earlier   Later