| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-24 | |||
| 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 | |
| 09:59:38 | sean-k-mooney | Uggla: https://github.com/openstack/oslo.db/blob/master/oslo_db/sqlalchemy/models.py#L129-L137 is where soft delete is implemented | |
| 10:00:12 | gibi | the ovo base class adds deleted_at column today | |
| 10:00:16 | gibi | to the ovo | |
| 10:00:23 | sean-k-mooney | to the object sent over the wire | |
| 10:00:36 | sean-k-mooney | but that does not mean it needs to be in the api reponce or the db model | |
| 10:01:23 | sean-k-mooney | we can just ignore them. if we want to have a new base class that does not have them that is also ok | |
| 10:01:25 | gibi | my original comment on Uggla's patch was to not hack out the deleted_at column from the ShareMapping ovo but not add it in the first place | |
| 10:01:50 | sean-k-mooney | ya im fine with that too | |
| 10:02:01 | gibi | https://review.opendev.org/c/openstack/nova/+/839401/6/nova/objects/share_mapping.py#49 | |
| 10:02:54 | sean-k-mooney | so they dont need to do that | |
| 10:03:05 | sean-k-mooney | if they just dont defien the columns in the db model | |
| 10:03:52 | gibi | honestly I don't want to to have columns in a new ovo that are not used | |
| 10:04:05 | sean-k-mooney | ack | |