| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-24 | |||
| 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 | |
| 10:04:18 | sean-k-mooney | we have that alredy for many object | |
| 10:04:37 | gibi | yes, but don't increase that debt if not really necesary | |
| 10:04:44 | sean-k-mooney | sure | |
| 10:05:05 | sean-k-mooney | so i think we are agreed Uggla should add one new base class without those fields | |
| 10:05:09 | sean-k-mooney | and not touch any of the rest | |
| 10:05:38 | sean-k-mooney | and we can disucss at the ptg if we will have time to do object cleanup in AA or BB and the faith of shadow tables | |
| 10:05:55 | gibi | agreed | |
| 10:05:56 | gibi | :) | |
| 10:06:24 | Uggla | sounds good to me. thanks. | |
| 10:08:28 | gibi | sean-k-mooney: re pci: I have to retract my original statement about nova splitting pools per PF. It splitted in my original test due to numa node differences (the fixture creates numa automatically). So on the same numa node we create pools with multiple PFs (of PCI devs). But as I already confirmed that the scheduler logic can split a request over multiple pools I can (and will) cahnge the pooling | |
| 10:08:34 | gibi | logic not to merge pools from different PFs or PCI devs | |
| 10:09:37 | sean-k-mooney | ack | |
| 10:09:39 | gibi | I realized that when I created a host with 3 PCI devs. so numa0 got two | |
| 10:09:47 | sean-k-mooney | ah | |
| 10:09:55 | sean-k-mooney | ya so i know it did it for numa | |
| 10:10:02 | sean-k-mooney | but within a numa node i was not sure | |
| 10:10:11 | sean-k-mooney | i think it also splits for differnnt tags | |
| 10:10:15 | sean-k-mooney | i.e. physnets | |