| opendevreview | Pranali Deore proposed openstack/glance master: Add new add location api https://review.opendev.org/c/openstack/glance/+/881940 | 07:57 |
|---|---|---|
| opendevreview | Pranali Deore proposed openstack/glance master: Add new get location api https://review.opendev.org/c/openstack/glance/+/882498 | 08:01 |
| abhishekk | dansmith, o/ | 16:53 |
| abhishekk | whoami-rajat, around? | 16:54 |
| abhishekk | do you know whether consumer of location API (cinder/nova) does any post processing on response based on image state at their end. | 17:00 |
| dansmith | abhishekk: currently? I dunno, but this is going to be a code change, so I'm not sure it matters | 17:03 |
| abhishekk | yeah, that is also true | 17:03 |
| dansmith | abhishekk: for snapshot I don't think it matters if we fire off the location add and don't care that the image is in importing or "digesting" or whatever state | 17:03 |
| dansmith | also let me say I'm sorry to be so late on review for this | 17:04 |
| dansmith | things have been quite busy lately :/ | 17:04 |
| abhishekk | ++, import workflow sounds better | 17:04 |
| abhishekk | no problem | 17:05 |
| abhishekk | I think its better to get it right at first place it self | 17:05 |
| abhishekk | I was initially wanted to persist on having this async but latter we added performance impact on spec to highlight the issue | 17:06 |
| dansmith | so another aspect to that, | 17:07 |
| abhishekk | Even though we will use import workflow, performance impact will be there but it will not keep consumer hostage for waiting | 17:07 |
| dansmith | is that the whole point of the accelerated ceph/glance/nova snapshot stuff is that nova can freeze the guest for a very short time, snapshot at ceph, let it go, and then just "tell" glance about it | 17:07 |
| dansmith | if we now have to wait for glance to eat and digest the whole thing, we lose a lot of that benefit | 17:07 |
| dansmith | (i.e. by setting our timeout to 30m or something crazy like that) | 17:08 |
| abhishekk | ++ | 17:08 |
| dansmith | also, I should probably ask.. | 17:08 |
| dansmith | we likely want to *enable* the async hashing, but not require it before the image can be used | 17:09 |
| dansmith | meaning, if you're doing cloud-fork kinda things, you want to snapshot an instance and immediately create N copies of it | 17:09 |
| dansmith | enabling hashing will slow that down if we can't use the image immediately | 17:09 |
| dansmith | it's not critical (like delaying the snapshot itself) but might be worth thinking about | 17:10 |
| abhishekk | that will be configurable on consumer side, if they send do_secure_hash True then only we will calculate the hashing | 17:10 |
| dansmith | IIRC in the "import to multiple stores" workflow, once it's imported to one, the image is active and can be used and the rest of the uploads finish in the background | 17:11 |
| abhishekk | yes | 17:11 |
| dansmith | right but the (nova) user doesn't get to choose that | 17:11 |
| dansmith | so they can't say "I'd rather not have hashing because speed is important to me" | 17:11 |
| abhishekk | ok, then what is the use of having this do_secure_hash request param | 17:13 |
| dansmith | well, honestly, it's probably better to make that a glance config (default on), and then if they provide a hash, calculate and check, and if they don't provide one, calculate and store it | 17:13 |
| dansmith | do we have any other image states other than active where you can download the image? | 17:14 |
| abhishekk | nope | 17:14 |
| dansmith | okay | 17:15 |
| dansmith | well, I don't want to explode everything about the design, but it's just worth thinking about I think | 17:15 |
| dansmith | having to choose between "having a hash, which must be calculated before I use it" and "having a hash once it's available" is kindof a bummer | 17:16 |
| dansmith | if they provide a hash value when they add, then waiting to go active until it's validated makes sense | 17:16 |
| dansmith | if they don't, presumably it should go active immediately, because it's not going to fail anyway right? | 17:17 |
| dansmith | and nova won't provide a hash for snapshots because we don't know what it should be anyway, so .. straight to active in that case | 17:17 |
| abhishekk | yes that is what it will do now because we have do_secure_hash False by default | 17:18 |
| dansmith | ...go active immediately, but still calculate it | 17:18 |
| abhishekk | One active we can not set these properties I guess | 17:19 |
| abhishekk | *once | 17:19 |
| dansmith | that's a glance rule though (which we can fix) right? | 17:19 |
| abhishekk | :) | 17:19 |
| dansmith | maybe we should chat with rosmaita about that aspect | 17:20 |
| dansmith | if that's a fundamental design point, that we should never add a hash value after the image goes active, then I guess that's okay | 17:20 |
| dansmith | but it seems bad to block things on tasks that necessarily take a long time | 17:20 |
| abhishekk | I think that is the rule, | 17:22 |
| dansmith | but today that leads to never having hashes on snapshots which kinda sucks :/ | 17:23 |
| abhishekk | also as I pointed if nova never gonna send secure_hash and validation data then image will go to active state immediately | 17:23 |
| abhishekk | yeah | 17:23 |
| abhishekk | that is the point we are adding this | 17:23 |
| abhishekk | this also blocks us from caching these types of images since during caching it calculates checksum and fails in the end since the image doesn't has it | 17:24 |
| dansmith | ack | 17:25 |
| abhishekk | We do not allow setting os_hash_* and checksum using image-update | 17:25 |
| dansmith | from the outside that makes sense for sure | 17:26 |
| abhishekk | also while creating image there is no way to pass these images | 17:26 |
| abhishekk | these values to image | 17:26 |
| abhishekk | So somehow we need to make some compromise if we want to calculate the hash/checksum | 17:28 |
| dansmith | yeah I mean I think rosmaita has in the past expressed a desire to up the level of validation we're doing (either signatures or hashes) and I very much agree that we should be really airtight on that stuff | 17:34 |
| abhishekk | agree about later part | 17:34 |
| dansmith | upload and import are easy to calculate the right values. snapshots are just as important, they just need a different (async) workflow | 17:35 |
| abhishekk | So for this workflow we should make some amends? | 17:35 |
| dansmith | personally, I think allowing this workflow to go active immediately (if no hash is provided) and set the hash after the image is active would be best. But, if that's not acceptable, | 17:36 |
| dansmith | I'd rather the snapshots calculate the hash and just not go active until it's done | 17:36 |
| dansmith | i.e. break/delay the instant snapshot-and-boot "cloud fork" model | 17:36 |
| abhishekk | that is what we are currently doing but without async, if we move to import workflow we will be doing that | 17:37 |
| dansmith | yeah | 17:38 |
| abhishekk | but I think rosmaita will be around tomorrow and since we don't hve weekly meeting we can discuss this at that time | 17:38 |
| abhishekk | the possibility of 1st option | 17:38 |
| dansmith | ack | 17:38 |
| rosmaita | just got back, will read through the scrollback | 17:39 |
| abhishekk | just read from your 1st mention :D | 17:40 |
| whoami-rajat | abhishekk, when glance backend is cinder and we upload image, if the ADD location call fails/doesn't return success then we delete the cloned volume and fail the operation | 18:15 |
| whoami-rajat | abhishekk, so we kind of rely on the sync behavior of this workflow | 18:15 |
| whoami-rajat | https://github.com/openstack/cinder/blob/456b6399bece9a9ac6274e56130cff6680d99096/cinder/volume/manager.py#L1746-L1755 | 18:15 |
| abhishekk | you don't check for image state though | 18:16 |
| abhishekk | So if we decided to go option 1 then you will not be impacted | 18:17 |
| abhishekk | which is, , I think allowing this workflow to go active immediately (if no hash is provided) and set the hash after the image is active would be best. But, if that's not acceptable, | 18:17 |
| whoami-rajat | abhishekk, but what if the hash calculation fails? then the image state won't go to ACTIVE i.e. unusable image and we've a leftover cloned volume on cinder side | 18:18 |
| abhishekk | hmm | 18:18 |
| whoami-rajat | also the cinder operation will say the operation was successful with no error logs, then operators would need to search glance logs as to why image upload didn't work as expected | 18:19 |
| dansmith | whoami-rajat: will cinder provide the hash? | 18:19 |
| whoami-rajat | i.e. image upload was successful but glance image is queued | 18:19 |
| whoami-rajat | dansmith, there are 2 values that cinder can provide, do_secure_hash and validation_data which makes 3 cases possible (actually 4 but 4th one is the current behavior) | 18:20 |
| whoami-rajat | dansmith, https://specs.openstack.org/openstack/glance-specs/specs/2023.2/approved/glance/new-location-info-apis.html#rest-api-impact | 18:20 |
| dansmith | either way, anything that is sync for simplicity can be async with polling for robustness and you can handle it the same way if the image goes to error state as you would if you wanted 30m for one call to complete and got an error | 18:21 |
| dansmith | whoami-rajat: right, but I'm asking _if_ cinder will, and if it will, how does it do that without reading the whole volume? | 18:21 |
| abhishekk | polling side change at cinder makes sense | 18:21 |
| dansmith | if you had to clone the volume and upload it to a backing store then sure, but if you're doing it in ceph, it should be the same as nova - where you don't want to read the whole volume only to provide the hash right? | 18:21 |
| whoami-rajat | dansmith, ah ok, cinder won't actually provide the validation data, if the end user has it they can provide it via config or CLI | 18:22 |
| dansmith | exactly :) | 18:22 |
| dansmith | so in cinder's case it will never fail hash validation | 18:22 |
| dansmith | as in nova's | 18:22 |
| whoami-rajat | it shouldn't | 18:23 |
| dansmith | the only thing that would fail is if you provide like a http url that can't be read (or which fails during hash validation) | 18:23 |
| dansmith | glance should retry retry-able errors, and if it's fatal, then you can't download the image anyway | 18:24 |
| whoami-rajat | abhishekk, dansmith, on a higher level, the idea is | 18:24 |
| whoami-rajat | 1) abstract locations from end users | 18:24 |
| whoami-rajat | 2) provide operators/users ability to choose performance vs security | 18:24 |
| whoami-rajat | 3) create separate APIs for location operations to get more control with policies (service or owner checks) | 18:24 |
| dansmith | yep, my argument is that #2 should not require a 100% tradeoff | 18:24 |
| dansmith | if I "prefer" performance, I should get security one the hash is calculated | 18:25 |
| dansmith | *once | 18:25 |
| dansmith | if I upload an image and I know the hash and provide it, I'm choosing security (i.e. don't make this active until you validate it matches) | 18:25 |
| dansmith | if I don't, I either can't enforce the security during the add, or don't want to, but once the hash has been calculated, everything else should be able to validate subsequent operations on that hash | 18:26 |
| abhishekk | If we allow to set these properties to active image for this operation/workflow then we will have no problem on both sides | 18:27 |
| dansmith | correct | 18:27 |
| abhishekk | I don't think there will be any issue in allowing that unless rosmaita thinks otherwise | 18:28 |
| dansmith | whoami-rajat: going back to your "cinder won't provide the hash unless the user does" comment.. is there that sort of proxy where someone shows up to cinder with a volume image that gets passed through (with its hash) to glance | 18:28 |
| dansmith | ? | 18:28 |
| whoami-rajat | but those properties will be set by glance, maybe a background process calculating the hash but not allow it for an end user? | 18:28 |
| dansmith | whoami-rajat: that's what we're talking about yes.. not available to users, but the import background task will update them | 18:29 |
| abhishekk | yeah | 18:29 |
| whoami-rajat | dansmith, I don't mind that idea since I'm not implementing it :P | 18:29 |
| abhishekk | lol | 18:30 |
| dansmith | ack, well, let's aim for the best solution regardless :) | 18:30 |
| abhishekk | this will haunt you when you will implement consumer side :P | 18:30 |
| whoami-rajat | dansmith, sorry i didn't understand your previous question | 18:31 |
| dansmith | whoami-rajat: [11:22:29] <whoami-rajat> dansmith, ah ok, cinder won't actually provide the validation data, if the end user has it they can provide it via config or CLI | 18:31 |
| dansmith | whoami-rajat: did you mean just the glance api/cli or is there a user->cinder->glance workflow that involves providing the hash values ahead of time? | 18:31 |
| abhishekk | former part, glance api/cli can provide validation data for http store | 18:33 |
| dansmith | ack okay just making sure I wasn't missing some other flow | 18:33 |
| whoami-rajat | dansmith, so we've upload volume to glance operation on cinder side that creates an image with same data as the volume, we've a user facing CLI for it -- if that's what you are asking about | 18:33 |
| dansmith | whoami-rajat: right but the user wouldn't provide a hash value during that operation right? | 18:33 |
| abhishekk | cinder or nova can just provide do_secure_hash true if we keep it default fase | 18:34 |
| whoami-rajat | dansmith, right now they can't but if we want decide to pass validation data from cinder/nova to glance then we can add new parameters | 18:34 |
| whoami-rajat | dansmith, mainly it was for the http store case | 18:34 |
| whoami-rajat | I think nova/cinder will just use the do_secure_hash param | 18:34 |
| dansmith | whoami-rajat: ack, okay, well, I'm not sure how the cinder user would know the hash for the volume (without a very laborious effort) but yeah I get it | 18:35 |
| whoami-rajat | mainly s/it/validation_data was for http store case | 18:35 |
| dansmith | whoami-rajat: well, I think we should *always* hash (unless maybe disabled in config) | 18:35 |
| dansmith | whoami-rajat: and wait for active if you provide the values ahead of time, and not wait if you don't | 18:35 |
| whoami-rajat | dansmith, if we go with the async flow, we should always. we wanted to make it configurable for the performance impact of sync operation https://specs.openstack.org/openstack/glance-specs/specs/2023.2/approved/glance/new-location-info-apis.html#performance-impact | 18:35 |
| dansmith | so if you provide a hash, you're saying "don't let this be active until you validate that it is correct" and if you don't, you say "calculate the hash in the background and set it when it's available" | 18:36 |
| dansmith | whoami-rajat: ack yeah | 18:36 |
| dansmith | the fact that we have no hashes for any of our snapshots today is really bad, IMHO, and this is an opportunity to fix that past mistake :) | 18:36 |
| abhishekk | for this we all are on same page :D | 18:37 |
| whoami-rajat | dansmith, yes that understanding looks correct | 18:37 |
| dansmith | ++ | 18:37 |
| whoami-rajat | if jokke_ is around, he would be happy to know we are finally fixing one of the issues he used to mention in every cinder glance cross project :D | 18:38 |
| dansmith | lack of hashes? | 18:38 |
| whoami-rajat | i.e. the nova/cinder created images lack security | 18:38 |
| whoami-rajat | yes | 18:38 |
| dansmith | yeah, it's almost embarrassing to be missing those :) | 18:38 |
| dansmith | s/almost// | 18:38 |
| whoami-rajat | yep, thanks dansmith for taking a look | 18:39 |
| dansmith | np, sorry it's so late :) | 18:39 |
| abhishekk | So in import workflow, state will be queued, active if validation data not provided but do secure hash is true, else it will be queued,importing,active | 18:39 |
| dansmith | just drop do_secure_hash, IMHO | 18:39 |
| dansmith | make it config, default on | 18:39 |
| dansmith | operators can disable it for PoC or silly situations, but don't let the user choose per request | 18:40 |
| whoami-rajat | yeah agreed, if it's async and image will be active, cinder/nova won't mind glance doing extra work | 18:40 |
| whoami-rajat | :D | 18:40 |
| dansmith | hah, right :) | 18:40 |
| dansmith | makes it simpler that way | 18:41 |
| dansmith | harder to mess up | 18:41 |
| dansmith | and, if we get this in place, you could make the glanceclient stop agreeing to download images with no hash unless --download-insecure is passed or something | 18:41 |
| dansmith | then people can decide if they want to take the image while the hash is being calculated or not | 18:41 |
| abhishekk | hmm, that will be good improvement as well | 18:43 |
| whoami-rajat | one question, if the hash calculation fails, we will just won't update the hash in image right? or we will "deactivate" or "queue" the image again? | 18:43 |
| dansmith | whoami-rajat: fails because it didn't validate against the provided one, or failed due to network or something else? | 18:43 |
| abhishekk | if validatation data is provided then we don't set image to active immediately | 18:44 |
| abhishekk | it will be in importing state | 18:44 |
| dansmith | right | 18:44 |
| abhishekk | if validation fails it will be rolled back to queued if succeeds then active | 18:44 |
| dansmith | if we fail to download the image (like via http) in order to calculate the hash, I think we should retry a few times and then fail the image | 18:44 |
| dansmith | because it won't likely be downloadable anyway | 18:44 |
| abhishekk | yep | 18:46 |
| whoami-rajat | dansmith, abhishekk I'm referring to the case when we don't provide validation data. we set the image to active and do the hash calculation in background. but if someone uses that image to create VMs or bootable volumes and later the hash calculation fails and we move the image to QUEUED, what will happen to those VMs and volumes? | 18:46 |
| abhishekk | how will hash calculation fail unless service down or something else? | 18:47 |
| dansmith | abhishekk: right | 18:47 |
| dansmith | I would put it to error personally | 18:47 |
| whoami-rajat | the process gets killed due to OOM? there could be several cases since it's a CPU intensive task | 18:47 |
| whoami-rajat | dansmith, but we've already created resources from that image ... | 18:48 |
| dansmith | whoami-rajat: OOMing from a hash calculation would mean something is *very* wrong :) | 18:48 |
| dansmith | whoami-rajat: well, that can happen today if you boot an instance from a snapshot and then delete the image | 18:49 |
| whoami-rajat | dansmith, don't we keep chunks in memory? and chunk size is configurable? sorry if I'm ignorant towards the hash calculation logic | 18:49 |
| whoami-rajat | dansmith, but in cinder case, if glance and cinder are both using ceph, bootable volumes are COW clones of image i.e. dependent on the image | 18:49 |
| dansmith | whoami-rajat: we should not need more than one chunk's worth of memory at any one time.. 4MB at the *most* I would think, 64k in most other cases.. hashes are very favorable to streaming calculation | 18:50 |
| whoami-rajat | ok | 18:50 |
| dansmith | whoami-rajat: ack, well, we could just leave the hash value unset, but I personally don't think that's right | 18:50 |
| dansmith | whoami-rajat: perhaps some way to re-trigger hash calculation once the issue is fixed or something | 18:51 |
| whoami-rajat | dansmith, so i just want to brainstorm the cases so we don't end up creating a regression in current operations | 18:51 |
| abhishekk | I think it will be better to leave it unset in worst possibility | 18:51 |
| dansmith | whoami-rajat: ack, yep | 18:52 |
| dansmith | okay, then leave it unset after several retries, with a LOG.error() explaining the problem | 18:52 |
| abhishekk | several? | 18:52 |
| abhishekk | that will introduce another config option :D | 18:53 |
| abhishekk | or 3 times hardcoded | 18:53 |
| whoami-rajat | retry + unset (eventually) works for me, it's the same thing we have today, which is not great but at least works | 18:53 |
| abhishekk | its win win for all | 18:53 |
| dansmith | abhishekk: we probably need something like "http_retries" which could apply to this and other things with web-download right/ | 18:54 |
| whoami-rajat | though i think everyone has confidence that hash calculation shouldn't fail unless the deployment is in a really bad state | 18:54 |
| abhishekk | dansmith, yes, later we can use if to enhance web-download | 18:54 |
| dansmith | whoami-rajat: yep | 18:55 |
| dansmith | abhishekk: yep | 18:55 |
| dansmith | abhishekk: can we use taskflow to restart retry-failed tasks on glance-api restart or something? I think we don't persist the state, IIRC | 18:56 |
| abhishekk | no we don't | 18:57 |
| dansmith | ack | 18:57 |
| abhishekk | we are waiting to confirm from rosmaita about the approach whether to set checksum/hash on active image or not | 19:00 |
| * abhishekk signing off now, will be back tomorrow | 19:00 | |
| dansmith | o/ | 19:02 |
| abhishekk | o/~ | 19:14 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!