| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-12-09 | |||
| 09:57:24 | sean-k-mooney | gibi: chanig the api behavior would be just to have partity with the servers endpoint | |
| 09:57:25 | gibi | I don't need the API behavior to change | |
| 09:57:46 | gibi | I think it make sense that the history of a VM overlives the VM itself | |
| 09:58:03 | gibi | outlive :D | |
| 09:58:08 | sean-k-mooney | for server list you need to pass --deleted to see the soft deleted ones | |
| 09:58:14 | sean-k-mooney | you dont have to do that for show | |
| 09:58:25 | sean-k-mooney | as far asim aware jsut the list endpoint | |
| 09:59:02 | sean-k-mooney | anyway its fine do we still need a spec if we are not changing the api now | |
| 09:59:49 | sean-k-mooney | we will start soft deleting them in the db but we will change the api to ignore the deleted column | |
| 10:00:21 | sean-k-mooney | so the behavior is the same. | |
| 10:00:33 | gibi | do we need to add a data migration to fill the deleted column of existing actions? | |
| 10:00:53 | gibi | maybe based on the the fact if the instance is deleted or not yet | |
| 10:03:27 | sean-k-mooney | i dont think we do | |
| 10:03:37 | sean-k-mooney | we can just remvoe the deleted column form the query | |
| 10:03:57 | gibi | OK, then I think we don't need a spec, but I let bauzas to decide with his PTL hat | |
| 10:03:58 | sean-k-mooney | we can have an optional nova-manage command to fill it in | |
| 10:04:13 | sean-k-mooney | but i dont think it should be required | |
| 10:05:04 | bauzas | gibi: my first comment was to say : OK for a specless BP *but* not for the last phrase | |
| 10:06:03 | bauzas | https://meetings.opendev.org/meetings/nova/2021/nova.2021-12-07-16.00.log.html#l-203 | |
| 10:07:17 | gibi | <bauzas> "We should also implement the possibility to retrieve deleted instance actions as we do for instances." seems like an API change, right? | |
| 10:07:27 | gibi | but there our answer is to keep the API as is | |
| 10:07:43 | gibi | that fullfills this requirement without any API behavior change | |
| 10:07:59 | gibi | as the API always returns both deleted and not delete actions :) | |
| 10:08:25 | gibi | if the requirement is to retrive deleted instance actions _only_ | |
| 10:08:29 | gibi | then there is an API change | |
| 10:10:40 | sean-k-mooney | gibi: ya i dont think there really is a usecase fo deleted _only_ | |
| 10:11:04 | sean-k-mooney | so i think we are ok to not change the db | |
| 10:11:07 | sean-k-mooney | *api | |
| 10:11:27 | gibi | yepp, I'm on that side yesa | |
| 10:11:43 | sean-k-mooney | so the cahnge is really to two things | |
| 10:12:03 | sean-k-mooney | the server delete sql query/funciton will need to soft delete the instance actions | |
| 10:12:23 | sean-k-mooney | and the instance actions quies will need to ignore the deleted field | |
| 10:12:29 | sean-k-mooney | and that shoudl be sufficnet | |
| 10:12:46 | pslestang | bauzas: I can change the BP to remove the possibility to retrieve deleted instance action as the solution proposed does not need such feature | |
| 10:12:57 | sean-k-mooney | oh and server restore shoudl also restore the instance_actions | |
| 10:13:05 | bauzas | pslestang: okay then | |
| 10:14:45 | pslestang | bauzas: done | |
| 10:15:43 | bauzas | gibi: sean-k-mooney: would we need to modify the DB if we accept to soft-delete the instance actions ? | |
| 10:16:05 | bauzas | pslestang: maybe that's here we would need to discuss about upgrades | |
| 10:16:15 | gibi | bauzas: we need to modify the DB yes, and need to modify the DB query the API does to ignore the new column | |
| 10:16:37 | bauzas | that's my question | |
| 10:16:47 | gibi | so if you are strict then there is an upgrade impact, as the db schema changes | |
| 10:16:47 | bauzas | for upgrades | |
| 10:17:10 | bauzas | I mean | |
| 10:17:18 | bauzas | for upgrading there are two directioins | |
| 10:17:34 | bauzas | 1/ we want to modify the DB for already existed instance actions | |
| 10:17:51 | bauzas | 2/ we don't do it and we only soft-delete the new instance actions | |
| 10:18:08 | bauzas | 1/ would mean that we would need to have a nova-manage command | |
| 10:18:27 | bauzas | 2/ wouldn't | |
| 10:18:53 | bauzas | for both, we need to discuss how the API query would do | |
| 10:19:01 | bauzas | remember that we have cells v2 | |
| 10:19:22 | bauzas | so we would need to know whether the cell DB is upgraded or not | |
| 10:19:33 | bauzas | before calling the instance actions table | |
| 10:19:41 | bauzas | (from the API I mean) | |
| 10:20:04 | bauzas | anyway, I need to get my kid from school in a few mins | |
| 10:20:27 | bauzas | but I guess we probably need to want to discuss about the upgrade questions in some... spec ? | |
| 10:20:45 | gibi | bauzas: that is your call :) I'm happy to discuss this in a spec | |
| 10:21:01 | bauzas | I don't wanna use my baton | |
| 10:21:08 | bauzas | it's more a core question | |
| 10:21:18 | bauzas | about upgrades | |
| 10:21:19 | gibi | regarding nova-manage that feels optional to me, and probably pslestang can state if OVH needs it or not | |
| 10:21:30 | gibi | so this is a requirement question | |
| 10:21:34 | bauzas | yeah | |
| 10:21:36 | pslestang | bauzas: by the way we already need to change the nova-manage purge behaviour which actually rely on update_at column for instance_action_* tables instead of deleted_at | |
| 10:21:44 | bauzas | correct | |
| 10:22:08 | bauzas | we also need to look at which methods look at instance actions | |
| 10:22:23 | bauzas | we know the API for sure | |
| 10:22:40 | bauzas | but I wonder whether we also look at the actions within nova directly | |
| 10:22:52 | sean-k-mooney | bauzas: we dont need to modify the db but we do need to modify hte db queries | |
| 10:22:59 | sean-k-mooney | the schema will remain the same | |
| 10:23:15 | bauzas | sean-k-mooney: agreed this isn't a DB schema modifcatioin | |
| 10:23:19 | gibi | sean-k-mooney: ooh the same already has deleted column, I missed that | |
| 10:23:20 | bauzas | but this is about the values | |
| 10:23:40 | sean-k-mooney | gibi: yep it already inherits form teh softdelete mixin | |
| 10:23:40 | pslestang | gibi: at OVH we do not use nova-manage but it could be useful for someone | |
| 10:23:41 | bauzas | for the moment, soft-deleted instances have not deleted actions | |
| 10:23:51 | sean-k-mooney | gibi: we just currently dont soft delete it | |
| 10:24:10 | bauzas | if we start to soft-delete actions when deleting instances, then I wonder what happens for existing instance related actions | |
| 10:24:26 | sean-k-mooney | the remain not deleted | |
| 10:24:30 | sean-k-mooney | but that wont mater | |
| 10:24:40 | bauzas | sean-k-mooney: this is one direction | |
| 10:24:45 | sean-k-mooney | since we will just start ignoring that field | |
| 10:24:47 | bauzas | or one solution | |
| 10:25:02 | sean-k-mooney | well setting a colume to the value it currently has in an update is valid | |
| 10:25:06 | bauzas | anyway, needs to get my daughter, moving | |
| 10:25:16 | sean-k-mooney | so the resotre can ignore it too and just always set it to 0 | |
| 10:25:34 | sean-k-mooney | even if its already 0 becuase we did not migrate the existing ones to eb soft deleted | |
| 10:26:24 | sean-k-mooney | pslestang: wehn you archive the deleted rows it will delete the instance actions | |
| 10:26:55 | sean-k-mooney | so by the time you get to purge they shoudl already be gone form the main table | |
| 10:27:41 | sean-k-mooney | archiveing the delete rows will remove the instance form the main table which would break the forien key constraitnt on the isntnace action table | |
| 10:28:08 | sean-k-mooney | so the rows have to be removed to archive the instance to the shadow tables | |
| 10:28:10 | pslestang | sean-k-mooney: from what I saw when you archive the deleted rows it does not delete the instance actions but copy them in shadow tables | |
| 10:28:37 | sean-k-mooney | it shoudl delete them form the main table and copy them to the shadow table | |
| 10:28:44 | sean-k-mooney | basically a move | |
| 10:28:49 | pslestang | yes that's it | |
| 10:29:00 | sean-k-mooney | yep that is the expect behavior | |
| 10:29:19 | sean-k-mooney | and pruge should remove them for the shadow tabels | |
| 10:29:43 | pslestang | and when we purge de archived rows, it deletes the instances based on deleted_at date, and deleted the insatnce_action bases on updated_at date | |
| 10:30:13 | sean-k-mooney | if you are using --before yes | |
| 10:30:27 | pslestang | yep | |