Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-10-05
11:55:08 sean-k-mooney if we had already disabeld shadow tabels or remvoe them i think we could do that
11:55:16 stephenfin sean-k-mooney: Aren't those different things?
11:55:32 sean-k-mooney there are 3 things
11:55:33 stephenfin soft-deleting an instance vs soft-deleteable tables
11:56:03 sean-k-mooney soft deleteing an instnace marks the row as deelte but does not actully delete it until the soft delete timeout expires
11:56:12 stephenfin I don't think it does
11:56:50 sean-k-mooney i woudl have to check but as far as im aware it at least updates teh state so that it nolonger shows up in insntace list
11:56:58 sean-k-mooney unless you add --deleted
11:57:18 sean-k-mooney if it truely is independed and not marking it as delete in the db
11:57:32 stephenfin https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L3363-L3377
11:57:44 sean-k-mooney then yes the 3 things (shadown tabels, soft deletable instance and soft deletable rows) would be independent
11:58:12 stephenfin it seems to be setting power_state, vm_state and task_state fields. It's not setting 'deleted' or 'deleted_at'
11:58:14 sean-k-mooney ok so its via a vm state
11:58:21 stephenfin yup
11:58:28 stephenfin which this doesn't touch
11:58:32 sean-k-mooney ok then ya no api impact so
11:58:44 sean-k-mooney add it as a ptg topic
11:59:53 sean-k-mooney so long term i like to remvoe nova-manage acive_delete_rows and make pruge act like purge in other project
12:00:12 sean-k-mooney in other projects purge it deletes the soft deleted rows
12:00:43 sean-k-mooney but if we can also remvoe those (optionally or eventually permently) i woudl be happy to do that too
12:01:36 sean-k-mooney stephenfin: i think i have mentioned this to you before but i woudl prefer if we mvoed to recommendign peole use https://github.com/ovh/osarchiver/
12:02:54 sean-k-mooney so if they have audit usecases they woudl enable soft-delete but use osarchiver to move them to there archival storage and out of our db
12:03:37 sean-k-mooney if you can do that we dont need shadow tabels. if you dont have that usecase then you can disabel the soft-delete feature
12:04:02 sean-k-mooney and upstream in B or C we could defautl to disabling it.
12:05:58 stephenfin added to the agenda
12:08:09 sean-k-mooney cool im goign to summeries my toughts on the gerrit commit
12:16:55 sean-k-mooney ok done https://review.opendev.org/c/openstack/nova/+/860401/1#message-6ff04f04d84a33ff65f08e6c3956ea194a2a915c hopefully that makes sense
12:24:52 stephenfin lgtm
12:43:23 sean-k-mooney i like the idea of doing this in oslo too by the way but ya either could work
15:32:26 stephenfin Uggla: I figured out what was wrong with https://review.opendev.org/c/openstack/nova/+/854355/ Do you mind if I push my changes?
15:32:31 stephenfin I'm leaving a comment now too
15:33:14 Uggla stephenfin, cool, no pb.
15:33:48 Uggla stephenfin, btw did you have time to the openstacksdk patch ?
15:34:01 stephenfin Yeah, I think I +2d it
15:34:10 Uggla \o/
15:40:44 opendevreview Stephen Finucane proposed openstack/nova master: objects: Add NovaSoftDeleteObject mixin https://review.opendev.org/c/openstack/nova/+/854355
15:40:50 stephenfin Uggla: lmk what you think ^
15:46:14 Uggla stephenfin, i'll have a look after our meeting.
15:47:23 opendevreview Stephen Finucane proposed openstack/nova master: rpc: Mark attributes as private https://review.opendev.org/c/openstack/nova/+/792803
15:55:59 melwitt kashyap: the "abort live migration if monitoring fails" patch was to fail in a proper way when the error is encountered, there is another patch that needs review that will do the actual ignoring of the particular error https://review.opendev.org/c/openstack/nova/+/852002
15:59:50 melwitt kashyap: there was an issue with the patch I had proposed to workaround it, so I abandoned it. ^ is the new one from another contributor
16:02:04 Uggla stephenfin, what you did in https://review.opendev.org/c/openstack/nova/+/854355/4..5, sounds good to me. Thank you. Now let's check if gibi, dansmith, bauzas agree.
16:03:11 bauzas I thought we said we haven't wanted to have API tables to be soft-deletable
16:03:36 bauzas but, we haven't said "yeah, we should deprecate the other tables"
16:05:16 dansmith yeah
16:05:28 dansmith I don't agree with the use of "deprecated" here
16:05:42 dansmith "not recommended for everything by default" makes sense
16:07:01 bauzas at least I'm afraid of saying "we deprecate instance record soft-deletion"
16:07:17 dansmith was there some decision to deprecate and actually remove this stuff? because I think I disagree with that
16:07:23 dansmith and if not, we should change the wording in the patch I think
16:11:34 bauzas Uggla: I looked at your patch
16:11:51 kashyap melwitt: Thanks for jogging my memory! I now recall
16:12:01 bauzas sounds quite good to me if you say 'we need to formally name which tables do softdelete"
16:12:16 bauzas which is what you code
16:12:19 kashyap melwitt: I thought this one from Brett has already merged...but apparently not yet. Is it waiting on something still?
16:12:43 melwitt kashyap: just needs a second reviewer
16:13:00 kashyap Ah, nod. I thought something else besides it.
16:13:02 melwitt I already +2ed it
16:13:09 melwitt nah
16:14:01 kashyap gibi: or any other core who's not Mel, can you please put this through? - https://review.opendev.org/c/openstack/nova/+/852002
16:14:48 kashyap melwitt: Also thank you for - https://review.opendev.org/c/openstack/nova/+/859358/1
16:15:44 melwitt :)
16:15:52 kashyap Often these unit tests take a ton of time (at least for me), and I keep duking around them
16:15:57 gibi kashyap: I added to my queue but no promises when I get to it
16:16:59 kashyap gibi: What? I thought you'd attach a promiese-to-be-executed-on-this-date to all your reviews!
16:17:20 melwitt kashyap: they take a ton of time for me, pretty much never goes smoothly 😆
16:18:06 kashyap melwitt: Good to know; I feel particularly low when a unit test that I'm struggling with takes so long that a hen will develop teeth, but the test won't come out right.
16:19:59 melwitt kashyap: "hen develop teeth" haha I've never heard that before
16:21:00 kashyap I recently learnt the expression, "as rare as hen's teeth" -- https://en.wiktionary.org/wiki/rare_as_hen%27s_teeth :P
16:22:54 melwitt it's funny :)
16:24:25 kashyap On that note, /me goes to make "non-hen" dinner
16:25:01 melwitt o/
16:33:46 gibi kashyap: :)
16:36:49 opendevreview Stephen Finucane proposed openstack/nova master: objects: Use imports instead of type aliases https://review.opendev.org/c/openstack/nova/+/738018
16:36:49 opendevreview Stephen Finucane proposed openstack/nova master: objects: Remove unnecessary type aliases, exceptions https://review.opendev.org/c/openstack/nova/+/738240
16:36:50 opendevreview Stephen Finucane proposed openstack/nova master: objects: Remove 'NovaObjectDictCompat' from 'Service' https://review.opendev.org/c/openstack/nova/+/835595
16:36:50 opendevreview Stephen Finucane proposed openstack/nova master: objects: Remove wrappers around ovo mixins https://review.opendev.org/c/openstack/nova/+/738019
16:36:51 opendevreview Stephen Finucane proposed openstack/nova master: WIP: add ovo-mypy-plugin to type hinting o.vos https://review.opendev.org/c/openstack/nova/+/758851
17:29:41 opendevreview Stephen Finucane proposed openstack/nova master: objects: Add NovaSoftDeleteObject mixin https://review.opendev.org/c/openstack/nova/+/854355
17:30:54 sean-k-mooney stephenfin: to be clear the goal of ^ shoudl be the minimal possible change to allow Uggla to create teh manilla share object models without delete/deleted at in the db or ovo
17:33:42 stephenfin Yup, I think that's doing that? I suspect dansmith and bauzas had missed part of the commit message that explained that
17:34:06 stephenfin "Currently, the NovaPersistentObject mixin includes fields required by the soft delete feature - deleted and deleted_at - even if the backing SQLAlchemy model isn't using soft delete."
17:34:20 stephenfin 👆 that bit
17:35:03 sean-k-mooney right but other way to do that is for the manilla object to inherit form the timestamed one
17:35:08 sean-k-mooney and make no change to anything else
17:36:05 sean-k-mooney so instead of inheriting form NovaPersistentObject the manila share ones could inherit form ovoo_base.TimestampedObject which is also called NovaTimestampObject
17:36:23 sean-k-mooney we dont need to add the mixin for the orgianl usecse
17:36:49 stephenfin Same thing, different approach. If I was Uggla, I'd probably do that to avoid this blocking things
17:36:58 sean-k-mooney gibi: Uggla did i miss why we are not just doing that
17:37:35 sean-k-mooney stephenfin: thats basically what i suggested a month ago https://review.opendev.org/c/openstack/nova/+/854355/5#message-839b7637eabf1d6dcf4e95050e9244c954dc1056
17:37:55 sean-k-mooney at that time i did not see that we had NovaTimestampObject
17:38:03 sean-k-mooney and did not need ot intoduce a new class at all
17:38:43 stephenfin That change does still make sense though. As unlikely as it is that anyone will bump those major versions, it is confusing and the TODOs are helpful to highlight that
17:39:15 sean-k-mooney i dont dissagree that we might also want to do this
17:39:24 sean-k-mooney but i dont think we want to do this in the manilla share seriese
17:39:30 stephenfin agreed
17:39:39 sean-k-mooney which si why i ask Uggla to not do this when i first reviewed
17:40:34 gibi I cannot recall the reason we went that way
17:40:47 gibi probably to fix the other ovos where we can fix
17:41:01 sean-k-mooney gibi: Uggla orgingial patch alredy had updated all the other objects

Earlier   Later