Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-24
05:26:37 auniyal__ please review these
05:26:39 auniyal__ https://review.opendev.org/c/openstack/nova/+/853811
05:26:53 auniyal__ https://review.opendev.org/c/openstack/nova/+/853812
06:46:09 crohmann Hey lovely nova folks. I was just about to raise a bug about duplicate indices for tables of Nova and Placement, but then found an old, but unfixed bug: https://bugs.launchpad.net/nova/+bug/1641185
06:47:58 crohmann Since this is already assigned to ABHAY (since 2018) I believe this might be under the radar. Any chance this could be reassigned or place onto the list of "open isuses" ?
06:52:33 crohmann This also appears to have a simple fix in removing the double definitions of colums as primary indexes as well as them having a unique constraint.
07:21:37 gibi crohmann: hi! thanks for checking before reporting a new bug. Do you plan to proposa a fix?
07:22:07 gibi if so, then feel free to reassing the bug
07:22:35 gibi (or I can reassing it to you if you don't have the rights)
07:39:53 crohmann gibi: I did assign me and also raised a bug for placement at https://storyboard.openstack.org/#!/story/2010251.
07:40:45 crohmann Regarding a fix ... I suppose there are two sides: Fixing the schema for new installs, but also dropping them for existing ones, right?
07:41:04 crohmann "them" = the duplicate index
07:44:52 gibi crohmann: you are correct
07:45:41 gibi you need to drop it from the schema and also propose a db schema migration to drop it from existing dbs during upgrade
09:04:38 opendevreview Gorka Eguileor proposed openstack/nova master: Support os-brick specific lock_path https://review.opendev.org/c/openstack/nova/+/849328
09:08:41 opendevreview Gorka Eguileor proposed openstack/nova master: Support os-brick specific lock_path https://review.opendev.org/c/openstack/nova/+/849328
09:11:22 opendevreview Amit Uniyal proposed openstack/nova master: Adds check for VM snapshot fail while quiesce https://review.opendev.org/c/openstack/nova/+/852171
09:15:37 sean-k-mooney the unique key is the one that should be kept
09:19:29 opendevreview Balazs Gibizer proposed openstack/nova master: Support move operations with PCI tracking in placement https://review.opendev.org/c/openstack/nova/+/854247
09:20:45 opendevreview ribaudr proposed openstack/nova master: Default Nova persistent objects without soft delete. https://review.opendev.org/c/openstack/nova/+/854355
09:23:16 sean-k-mooney gibi: since the update userdata feature will need a new trait and a new os-traits reelase i think we shoudl swap the microverions for that and rebuild
09:23:32 Uggla gibi, I did that change ^ then I want to modify act objects like aggregate that do not requires soft delete. Unfortunately it changes the API output. So I guess we need a new microversion at minimum. But do we need a "deprecation" cycle too ?
09:24:01 sean-k-mooney Uggla: waht are you chagining
09:24:30 gibi sean-k-mooney: we have "time" until friday to push an os-traits change. But if the rebuild series is ready then I have no objection to swap
09:25:02 sean-k-mooney i have not reviewed it so i cant say
09:25:25 Uggla sean-k-mooney, https://review.opendev.org/c/openstack/nova/+/854355 trying to make persistent objects without soft delete "by default"
09:25:35 gibi Uggla: let me look at it. For sean-k-mooney, the new ShareMapping object needs to be non-soft-deletable and that needs some new baseclass for Nova ovos as the current one adds the deleted_at field
09:26:06 sean-k-mooney right but we dont need to change all the others
09:26:11 gibi sean-k-mooney: re rebuild series: me neither so for me both userdata and rebuild is in the grey zone but if somebody says that rebuild is ready to land then I'm OK to swap
09:26:17 sean-k-mooney we could eventually
09:26:31 gibi sean-k-mooney: re ovo: yes, we only need to change the base class for the new ShareMapping
09:27:26 sean-k-mooney right so https://review.opendev.org/c/openstack/nova/+/854355/1/nova/objects/base.py#142 is wrong
09:27:43 sean-k-mooney we shoudl not modify the NovaPersistentObject
09:28:02 sean-k-mooney we shoudl leage that the same and add a seperate one that does not use soft delete
09:28:31 gibi ^^ agree
09:29:06 sean-k-mooney maybe call it NovaPersistentObjectHardDelete for now
09:30:04 gibi wondering that the problem is only that the base class change changes the object signature used for versioning
09:30:31 sean-k-mooney it will change the ovo shas
09:30:41 Uggla sean-k-mooney, gibi it means we will keep objects with the removal of delete deleted_at "tricks" forever ?
09:30:51 sean-k-mooney but if Uggla updated them all to point to NovaPersistentSoftDeleteObject it woudl be fine
09:31:13 sean-k-mooney Uggla: if we remove soft delete it will be a sperrate spec
09:31:14 Uggla sean-k-mooney, no the current changes does not change the shas
09:31:23 sean-k-mooney its not something you should do in your current one
09:33:46 gibi Uggla: then what is the exact API change you are worried about?
09:34:59 gibi Uggla: I don't see the reason why the aggregate API output would differ
09:35:06 gibi after your patch
09:35:23 gibi ohh
09:35:39 Uggla gibi, in another patch I wanted to clean the aggregate object
09:35:41 gibi so the aggregate is not soft deleted
09:35:46 Uggla yep
09:35:53 gibi leave that alone for now :)
09:36:22 gibi in your feature you add a new ovo which is not soft deletable, that is good
09:36:48 gibi then in AA if you want, you can work on bumping the Aggregate OVO to 2.0 and add a microversion to hide deleted_at
09:36:49 Uggla so I change it as a test to inherit from NovaPersistantObject (without soft delete)
09:38:21 Uggla gibi but is it something that could be done in AA or later to have a "deprecation" time ?
09:39:17 gibi there is multiple things. The API change needs a microversion bump, it does not need a deprecation period but we need to support old microversions( you can fake deleted_at in the API response for old microversions)
09:39:36 gibi the Aggregate OVO change needs a major ovo version bump to 2.0 to remove a field
09:39:52 sean-k-mooney same for all other objects
09:40:00 gibi that means if we pass Aggregate OVOs over RPC then we need to support Aggregate 1.x and 2.0 parallel for an extra release
09:40:23 sean-k-mooney we cant do the db contraction till CC
09:40:32 gibi yes, that is the 3rd thing
09:40:34 sean-k-mooney because of teh new life cycle
09:41:12 gibi but the Aggregate table has no deleted_at so no need to contract there
09:41:17 sean-k-mooney well technially we can do it in BB if we deperecate teh soft delete feature in AA
09:41:23 Uggla yes so do you think it worth it ?
09:41:43 sean-k-mooney yes long term but i can upgrade my -1 to a -2 if you like
09:41:57 sean-k-mooney you should not be doing this as part of the share change
09:42:06 sean-k-mooney its its own spereate thing
09:42:22 Uggla sean-k-mooney, no this is outside of the share stuff.
09:42:34 sean-k-mooney right it needs a spec
09:42:36 gibi Uggla: it depends. I think the important part is not to add new ovos with soft delete. Cleaning up the old ones is a bit of busy work in my eyes
09:43:05 sean-k-mooney it has a prerrty large upgrad impact so it cant reasonable be done as a bugfix or specless blueprint
09:43:28 sean-k-mooney gibi: this kind of need to be done in tandom with removign shadow tables for it to be useful
09:43:30 gibi yes I agree to draft a spec for it if you want to work on this
09:43:40 gibi sean-k-mooney: good point
09:44:23 Uggla gibi, agree I don't want to work on something if you think the benefits are low.
09:44:24 sean-k-mooney personally removing the shaddow tabels woudl be more useful in my view then the soft delete mechanisum
09:44:52 sean-k-mooney Uggla: its not that its low its that there is a lot more work to do then just that patch
09:45:22 gibi Uggla: you can bring this up on the PTG to get a wider set of opinions about it
09:45:37 sean-k-mooney Uggla: its quite a large change to do right and ensure we dotn break rooling upgrades
09:47:12 Uggla sean-k-mooney, agree I know that changing that is a lot of work due to the API impact etc... But from your point of view is it something that "bring value" ?
09:47:14 sean-k-mooney by the way if an object suppots soft delele is determin by if it has the softDeleteMixin in the db model
09:47:16 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/db/main/models.py#L139
09:47:19 sean-k-mooney not the ovo
09:47:39 sean-k-mooney Uggla: its something i have wanted to do for a few releases
09:48:20 sean-k-mooney basically i want to remove or custom db backup/audit facilaties sicne operator have written there own
09:48:45 sean-k-mooney nova i think is the only project with shadow tabels so they wroge a solution that worked for all of them
09:50:43 sean-k-mooney https://github.com/ovh/osarchiver/
09:50:45 sean-k-mooney ^
09:51:08 sean-k-mooney i woudl like to replace or in tree shadow tabels with that solution eventually
09:52:20 sean-k-mooney i would propose formally deprecating Shadow tabels in AA and remove them in BB
09:52:38 sean-k-mooney soft delete we might want to keep becuase you can undelete instnaces
09:52:50 sean-k-mooney if the soft delete functionlatiy is enbaled itn the config
09:56:06 sean-k-mooney Uggla: gibi by the way we do not support fog deleteing for the api db resouces
09:56:07 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/db/api/models.py#L61-L95
09:56:59 sean-k-mooney we do for all the cell db resouce excpt server tags and console auth tokens
09:57:01 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/db/main/models.py#L1023-L1073
09:57:05 gibi yep I noted that above that the aggregate db table has not deleted_at column so no need to contract
09:57:48 sean-k-mooney so for the share mappign we dont actully need a new ovo
09:58:02 sean-k-mooney we can just not add the soft delete mixing to the table in the db model

Earlier   Later