14:08:25 <jokke_> #startmeeting glance
14:08:26 <openstack> Meeting started Thu Jan 30 14:08:25 2020 UTC and is due to finish in 60 minutes.  The chair is jokke_. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:08:27 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:08:29 <openstack> The meeting name has been set to 'glance'
14:08:31 <jokke_> #topic roll-call
14:08:33 <jokke_> o/
14:08:38 <rosmaita> o/
14:08:46 <yebinama_> o/
14:09:16 <rosmaita> looks like a short agenda today, just open discussion
14:09:50 <rosmaita> i am behind on my reviewing, is there anything in particular that i should look at first?
14:10:44 <jokke_> #topic open discussion
14:11:33 <jokke_> ok, yebinama_ looking into the hashing
14:11:44 <jokke_> sorry for noticing it this late
14:11:58 <yebinama_> No problem
14:12:23 <yebinama_> Did you see my update?
14:12:27 <rosmaita> link to patch?
14:13:27 <yebinama_> review.opendev.org/#/c/667132
14:13:35 <rosmaita> ty
14:13:37 <jokke_> https://review.opendev.org/#/c/667132/21/glance/location.py
14:14:27 <jokke_> yebinama_: I saw your comment and that you had update, didn't have my head with me glancing it through to follow what was going on there :P
14:14:37 <jokke_> I need to look through it properly
14:14:51 <yebinama_> Ok :)
14:15:12 <yebinama_> I didn't do the revert part
14:15:23 <yebinama_> I had a question regarding it
14:15:26 <jokke_> yeah, that might get hairy
14:15:38 <jokke_> go for it rosmaita might be able to help as well
14:16:00 <yebinama_> Today, if an import fails we don't revert to queued state?
14:16:18 <yebinama_> Or I didn't see the code for this
14:17:20 <jokke_> IIRC it depends, I think web-download reverts to queued glance-direct might go to uploading or something like that as we leave the data in staging
14:17:42 <rosmaita> i think that's correct
14:17:48 <jokke_> but neither should leave it active with locations in there
14:18:51 <yebinama_> Ok so I don't have to change that part
14:19:32 <yebinama_> Just make sure in case of revert to delete os_hash ans other related
14:19:53 <jokke_> no, what I meant and might have frased it wrong was that when we rever to the point that we have no locations in the image, we should not have hashes there either
14:20:00 <jokke_> yep!
14:21:04 <jokke_> and this is the part that might get hairy, rosmaita do you remember if we have some protections at the db lever for those properties?
14:21:43 <rosmaita> you mean whether a user can modify them?
14:22:13 <rosmaita> they're protected however we protect 'checksum'
14:22:13 <jokke_> I mean wether we can actually internally remove them once they have been set
14:22:34 <rosmaita> that's a good question
14:22:43 <jokke_> as I can't remember which point of the code path those protections kicks in
14:23:26 <yebinama_> I think we can do it as with the code I wrote today, they are changed each time an import is done
14:23:32 <rosmaita> i think they get filtered at the api layer
14:23:47 <jokke_> yebinama_: that sounds great
14:23:47 <rosmaita> i don't think we have any hard core protections in the DB
14:23:55 <yebinama_> And that's what you asked to change
14:24:51 <jokke_> yebinama_: yeah, true that's where my initial concern came from so they are not protected on this level ;)
14:25:13 <jokke_> I really need to take my head with me next time I sit back to the computer :D
14:25:31 <yebinama_> :)
14:25:52 <rosmaita> sounds like we are OK thne
14:25:54 <jokke_> yebinama_: so it's clear for you now what I was looking for?
14:25:56 <rosmaita> *then
14:26:00 <yebinama_> My only concern is where is the revert code you talked about
14:26:17 <yebinama_> There is no revert in image import flow
14:26:21 <jokke_> and rosmaita does that make sense to you?
14:27:12 <rosmaita> you mean the part about if there's no image data, then os_hash_* and checksum should not be set?
14:28:40 <yebinama_> The part where we go back from active to queued
14:28:50 <jokke_> rosmaita: I mean the 2 cases, first we should not set the checksums every time we upload image, but rather validate against the first one if we do multi store import and if it fails we should revert the checksums and algo out of the image metadata as well when we clean up the locations
14:29:25 <rosmaita> i definitely agree with that
14:29:27 <jokke_> yebinama_: so each task in taskflow has it's rever function (if it's defined)
14:29:50 <jokke_> revert
14:30:42 <jokke_> taskflow calls those on failure (this happens each time there comes exception through the flow instead of sucessful return)
14:30:48 <yebinama_> Yep I added one for ImportToStore
14:31:37 <jokke_> so basically what we want to do is, in the code that deletes the locations upon failure, add a check that looks if we delete last location and clean out the chekcsum and _os_hash_*
14:31:40 <yebinama_> But you told that today if an import fails, glance set the image status back to queued
14:32:31 <yebinama_> I couldn't find that revert in the code
14:34:04 <jokke_> yebinama_: web_download.py L:132 does
14:34:30 <jokke_> so like said, we do it all the way to queued on the case of web-download method as we do not have data in staging
14:35:40 <yebinama_> Oh ok, not the image import. I missed that point.
14:36:09 <yebinama_> So I don't have to deal with it, thanks for the clarification.
14:37:04 <jokke_> sorry, you don't need to deal with what?
14:37:32 <yebinama_> Reverting the state in image import workflow
14:37:58 <jokke_> ah, no we don't need to change into which status we are reverting the image to
14:38:42 <yebinama_> Sorry for the confusion :)
14:39:25 <jokke_> np, it's good that we're on the same page. I love taskflow for what it enables us to do but It can be confusing
14:40:06 <yebinama_> Yes it' a great feature.
14:41:36 <jokke_> if you ever want to get few more grey hair, look our first try with it on the tasks api (the code lives directly under async_/flows/)
14:41:56 <jokke_> It was nice try, but the user experience was from the horror movies
14:42:25 <yebinama_> I'll give it a look :)
14:42:50 <jokke_> ok, we've spent 40min on this (which is good) ... anything else?
14:43:38 <yebinama_> It's good for me.
14:44:20 <jokke_> I'm good too, rosmaita?
14:44:49 <rosmaita> sounds good to me (given that i haven't looked at the code yet)
14:44:59 <rosmaita> i have something else
14:45:09 <jokke_> rosmaita: more like timecheck for the meeting
14:45:12 <jokke_> ah, exactly
14:45:17 <rosmaita> i wonder if we should deprecate 'checksum' this cycle
14:45:38 <rosmaita> i think the md5 reliance is becoming a problem
14:45:50 <rosmaita> both for us calculating it, but also for people confirming it
14:46:06 <rosmaita> i think the multihash has been in place for a few cycles
14:46:15 <rosmaita> hopefully people are starting to use that instead
14:46:44 <rosmaita> but, i admit that it's going to be a problem for legacy images
14:46:47 <jokke_> I'm even less worried it being there, but lots of environments are removing the support to actually calculate them
14:47:24 <rosmaita> yeah, so i guess we can leave it there, but stop computing it in Victoria
14:47:37 <rosmaita> that way we don't change the image-show response
14:48:22 <jokke_> need to see what abhishek thinks, but yeah I agree, we could potentially stop adding it, or at least write checks if md5 is not supported in the env, leaving it out
14:48:24 <rosmaita> also, i'm not sure how glanceclient would handle md5 being unavailable
14:48:50 <rosmaita> it uses multihash if its available
14:49:09 <rosmaita> but i think it will always try to compute md5sum
14:49:13 <jokke_> the nice part of md5 is that it's super light to calculate (that's why it's still used in so many places)
14:49:16 <rosmaita> although there is a really weird case in that code
14:50:06 <jokke_> it might calculate it but it definitely doesn't expect it to be there
14:50:26 <jokke_> as all the ceph snapshots comes without any checksums and they still work
14:51:24 <rosmaita> yeah, a null checksum is ok, i think the problem case will be checksum is there, no multihash, it will try to compute the checksum, get an exception, and then the image can't be downloaded
14:52:04 <jokke_> yeah, we need to look into the client code if we're finally dropping this for sure
14:53:48 <jokke_> ok, 7min left, something else?
14:54:08 <yebinama_> Just remembered something
14:54:12 <rosmaita> ok, i'll put it on the agenda for next week and we can see what abhishek thinks
14:54:24 <jokke_> rosmaita: gr8
14:54:30 <yebinama_> I check checksum and hash for each import
14:54:44 <yebinama_> But I also added a check for the size
14:54:53 <yebinama_> Is it needed or not?
14:55:43 <jokke_> not necessarily, but it's lightweight check and kind of makes sense there
14:55:59 <jokke_> I didn't mind it when I saw it
14:56:07 <yebinama_> Ok, I'll keep it then. Thanks.
14:56:13 <jokke_> if our size suddenly changes, something is definitely wrong
14:56:28 <jokke_> specially if the hashes matches
14:57:17 <rosmaita> no kidding
14:57:55 <jokke_> ok, last call, going first
14:58:06 <rosmaita> nothing from me, see you next week
14:58:21 <yebinama_> Bye.
14:58:22 <jokke_> ok, thanks all!
14:58:27 <jokke_> #endmeeting