| *** mhen_ is now known as mhen | 01:19 | |
| croelandt | #startmeeting glance | 14:00 |
|---|---|---|
| opendevmeet | Meeting started Thu Apr 24 14:00:20 2025 UTC and is due to finish in 60 minutes. The chair is croelandt. Information about MeetBot at http://wiki.debian.org/MeetBot. | 14:00 |
| opendevmeet | Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. | 14:00 |
| opendevmeet | The meeting name has been set to 'glance' | 14:00 |
| croelandt | #topic roll call | 14:00 |
| croelandt | #link https://etherpad.openstack.org/p/glance-team-meeting-agenda | 14:00 |
| abhishekk_ | Hi | 14:00 |
| mhen | o/ | 14:01 |
| rosmaita | o/ | 14:01 |
| croelandt | let's start | 14:01 |
| croelandt | I'll be chairing since mrjoshi is feeling unwell | 14:01 |
| croelandt | #topic Release/periodic job updates | 14:02 |
| abhishekk_ | Ok | 14:02 |
| croelandt | so glance.tests.functional.v2.test_images_import_locking.TestImageImportLocking.test_import_copy_bust_lock has been failing for a long time | 14:02 |
| croelandt | both in periodic jobs and on reviews | 14:02 |
| croelandt | I cannot reproduce this failure on my machine | 14:02 |
| croelandt | If anyone can, it would be helpful to figure out what's happening and maybe bisect the issue | 14:02 |
| croelandt | otherwise, I'm not sure what to do. Do we want to "temporarily" disable the test? | 14:03 |
| rosmaita | is it only failing on py310 ? | 14:04 |
| abhishekk_ | I don’t think disabling the test is a right option | 14:04 |
| croelandt | no, it's failing on multiple versions of Python | 14:04 |
| croelandt | but not always :) | 14:04 |
| croelandt | abhishekk_: so how do we debug that? | 14:05 |
| croelandt | I've tried running the failing test in a loop, and it *never* fails locally :/ | 14:05 |
| abhishekk_ | For time being | 14:05 |
| abhishekk_ | We can add tri catch and if it fails | 14:06 |
| rosmaita | do we randomize the test ordering? | 14:06 |
| croelandt | abhishekk_: and silently ignore the failure? | 14:07 |
| abhishekk_ | I think ao | 14:07 |
| croelandt | rosmaita: hm, how do we check that? | 14:07 |
| croelandt | it would be tox/stestr doing that, I guess? | 14:07 |
| rosmaita | yes, there | 14:07 |
| croelandt | hm do you know what config option turns the randomization on/off? | 14:08 |
| rosmaita | would be in tox.ini | 14:08 |
| abhishekk_ | Is it possible to run that test in the end? | 14:08 |
| rosmaita | looks like we do not randomize | 14:09 |
| croelandt | rosmaita: is it a good thing or a bad one? :) | 14:10 |
| rosmaita | though, to an extent you can get different ordering based on number of cpus available, i think | 14:10 |
| croelandt | yeah | 14:10 |
| croelandt | so "action items" :) | 14:10 |
| croelandt | abhishekk_: you'd want to try/except the failure? | 14:10 |
| rosmaita | well, it's good for this question, because it means that the failure isn't due to some test not cleaning up and then causing a problem | 14:11 |
| rosmaita | so probably the issue here isn't insufficient test isolation causing an issue | 14:11 |
| abhishekk_ | I need to check for that, I remember i had done it earlier for one test | 14:11 |
| croelandt | ok so maybe we force it to be first or last | 14:12 |
| croelandt | see if it keeps failing | 14:12 |
| abhishekk_ | It should be last imo | 14:12 |
| croelandt | I see "stestr run" has a "--load-list" option | 14:12 |
| croelandt | maybe listing all tests in there and rearranging them can do the trick | 14:13 |
| rosmaita | well, i think run stestr twice: first time, everything *except* the bad test, second time *only* the bad test | 14:13 |
| croelandt | oh I see | 14:14 |
| rosmaita | i think there's a "combine" option to stestr that you give the second one | 14:14 |
| croelandt | ok I'll send a DNM patch that does that | 14:14 |
| croelandt | yeah we can do "--exclude" for the first run, pass the test name for the second run, and combine | 14:14 |
| croelandt | ok good, let's move on | 14:15 |
| croelandt | #topic Important Zuul patches | 14:15 |
| croelandt | #link https://review.opendev.org/c/openstack/glance/+/933614 | 14:15 |
| croelandt | so this is an old patch of mine removing standalone deployments | 14:15 |
| croelandt | I don't remember the details to be honest :) But iirc devstack is no longer really using that, and without eventlet we'll drop that too | 14:16 |
| * dansmith stumbles in late | 14:16 | |
| croelandt | does it make sense to remove standalone deployments from our test runs? | 14:16 |
| dansmith | seems fine to me to remove standalone if we're trying to ... remove that ability :) | 14:17 |
| croelandt | yeah and I think that's in our plans | 14:17 |
| dansmith | croelandt: if you can link me to an example of the bust lock failure I can have a quick look at least | 14:18 |
| croelandt | oh it has *not* been merged indevstack | 14:18 |
| croelandt | https://review.opendev.org/c/openstack/devstack/+/932201 | 14:18 |
| rosmaita | https://56667f4336a8ddb79330-f6a340cbf02833d06cf13bf014ff46d4.ssl.cf1.rackcdn.com/openstack/e9a8cede349346b1a792b4a7af6ac4f7/testr_results.html | 14:18 |
| croelandt | rosmaita: thanks | 14:18 |
| rosmaita | dansmith: ^^ | 14:18 |
| dansmith | thanks | 14:18 |
| croelandt | so ok, maybe I'll check with stephenfin first | 14:18 |
| croelandt | abhishekk_: what do you think? | 14:19 |
| dansmith | croelandt: but the goal is to remove eventlet, which means removing standalone | 14:19 |
| rosmaita | i've looked at a few, seems to be the KeyError: 'os_glance_import_task' on the ones i've seen | 14:19 |
| dansmith | we could probably remove a bunch of eventlet code in glance by removing tandalone mode | 14:19 |
| croelandt | rosmaita: yeah it's always this one | 14:19 |
| croelandt | dansmith: then let's do it :) | 14:19 |
| abhishekk | ++ | 14:19 |
| croelandt | ok so I'll let you take a look at the zuul patch | 14:20 |
| croelandt | #topic One easy patch per core dev | 14:20 |
| croelandt | #link https://review.opendev.org/c/openstack/glance/+/944381 | 14:20 |
| croelandt | so this is about making sure we correctly update the doc headers every release | 14:20 |
| croelandt | but I also wonder whether we have a document listing all steps to take when making a release (send a patch to openstack/releases, update the doc headers, etc.) | 14:21 |
| croelandt | as PTL that would help | 14:21 |
| croelandt | otherwise I'll write down my own notes | 14:21 |
| abhishekk | +1 for doc | 14:21 |
| croelandt | yeah so if that does not exist I'll take notes and try to add something in doc/source/ by the end of the cycle | 14:22 |
| abhishekk | ack | 14:23 |
| croelandt | Moving on | 14:23 |
| croelandt | #topic Open Discussion | 14:23 |
| croelandt | So I'm gonna be away next two Thursdays | 14:23 |
| croelandt | oh and also the 29th | 14:23 |
| dansmith | should we chat about the image size transfer encoding thing? | 14:23 |
| croelandt | I think Mridula will be back to chair the meeting though | 14:24 |
| abhishekk | I want someone to have look at my scrubber patch | 14:24 |
| abhishekk | yes | 14:24 |
| croelandt | abhishekk_: yeah I need to take a look at the new version | 14:24 |
| rosmaita | croelandt: https://docs.openstack.org/glance/latest/contributor/releasecycle.html | 14:24 |
| croelandt | dansmith: you have the mic | 14:24 |
| croelandt | rosmaita: oh nice! | 14:24 |
| rosmaita | looks like it needs an update | 14:24 |
| dansmith | So I'm very much not in favor of disabling chunked transfers when someone passes image_size (or a flag to disable chunked transfers), especially since we've been doing chunked transfers for a long time, and we're about to freeze glanceclient | 14:25 |
| croelandt | rosmaita: yeah I'll read and update | 14:25 |
| dansmith | so I don't want to swoop in with a last-minute change to completely alter how we transfer gigs of image data, then freeze the library, | 14:25 |
| abhishekk | dansmith: in short we will be calculating size at client and then pass it to API | 14:25 |
| dansmith | especially since the right thing, IMHO, is to take image_size but send it to the API instead of changing the transfer encoding | 14:25 |
| abhishekk | AND at api side we will reject upload if size is not specified or continue without that? | 14:26 |
| dansmith | abhishekk: we can't calculate it, we need to take it as a parameter, but communicate it some other way to the API and not just change from chunked to unchunked transfer encoding | 14:26 |
| abhishekk | Ack, so user specified size we will rely on and then reject the whole upload if size mis-matches? | 14:27 |
| dansmith | it's not just the reject that matters, although we should do that too, | 14:27 |
| dansmith | it's that glance needs to know the size before the transfer starts so we can avoid resizing the backend every time we read a chunk | 14:27 |
| dansmith | to be clear, I'm referring to this: https://review.opendev.org/c/openstack/glance-specs/+/947423 | 14:28 |
| abhishekk | Sorry If I am missing something, last time I was under impression that we will know the filename and we can calculate the size based on that | 14:29 |
| dansmith | the original proposal was to add a "bypass chunked transfer" flag, which requires the user to know why that matters, when to use it, and why it's linked to the backend write behavior, which is wrong IMHO | 14:29 |
| dansmith | abhishekk: not always, if we pass you a generator you can't know (but nova knows) | 14:30 |
| abhishekk | ack | 14:30 |
| dansmith | the discussion has led towards more of a hack (IMHO) which is to bypass chunked encoding if we pass a size, which is just more obscure but the same problem, IMHO | 14:30 |
| dansmith | and it means the nova->glanceclient->glance->backend data pipeline we've been using for a long time will change subtly because we start passing the image size, | 14:31 |
| dansmith | only because glance requires it in content-length only | 14:31 |
| dansmith | so Im just trying to argue against changing all that data pipeline stuff, and linking the passing of image size to the fundamental way that all works, just because it's the "easiest" solution | 14:32 |
| abhishekk | We need to worry about traditional image upload scenario only I guess | 14:33 |
| dansmith | basically, if we do the "easy" solution it's only a patch to glanceclient, which can be backported easily (downstream) but has big ramifications to what's actually happening | 14:33 |
| dansmith | and I think that's a mistake | 14:33 |
| dansmith | abhishekk: probably.. I dunno how the plumbing works on import, but I suspect it has the size at that point, so everything is fine | 14:33 |
| dansmith | i.e. once it's in staging, we know the size | 14:34 |
| abhishekk | right, I am also assuming that we set size once image is staged | 14:34 |
| abhishekk | Need to confirm though I saw resize calls in logs on import as well | 14:35 |
| dansmith | if you use web-download, then it would not know the size and you'd see resizes | 14:35 |
| abhishekk | for web-download also we stage the image locally | 14:35 |
| dansmith | oh, I guess I forgot | 14:36 |
| abhishekk | yes we stage for all import operartions | 14:36 |
| dansmith | well, that's a different problem then | 14:36 |
| dansmith | maybe we set size during the import task flow then? if that's the case, then that's fixable internally fairly easily | 14:36 |
| abhishekk | agree | 14:37 |
| dansmith | but still, focusing on the thing in the spec, | 14:37 |
| abhishekk | ack, let me explain what I understand and see we are on right page | 14:37 |
| abhishekk | 1. Introduce --size option on upload (use existing if it is present), then pass it to API | 14:38 |
| abhishekk | 2. At API side if we don't have size continue the old way | 14:38 |
| abhishekk | 3. If we have size then compare it like hash at the end of upload and log warning and set correct size or reject the upload | 14:38 |
| dansmith | reject, but yes :) | 14:39 |
| abhishekk | Here at point 3, if user specified size as 38MB and actual size is 50MB then I guess it should fail for rbd backend as Initially we create volume of specified size right? | 14:40 |
| dansmith | yes | 14:40 |
| abhishekk | what will be the impact on other backends, specially swift, cinder and s3? | 14:40 |
| abhishekk | I guess cinder will also fail | 14:40 |
| dansmith | I think we can check the size in common code and make them all behave the same | 14:41 |
| dansmith | can, and should | 14:41 |
| dansmith | so will you rewrite the spec to say the 1,2,3 above? | 14:42 |
| abhishekk | ack | 14:42 |
| abhishekk | Yes, I will rewrite the spec | 14:42 |
| dansmith | cool thanks | 14:43 |
| abhishekk | mention current discussion as proposed change and the alternative as bypassing chunk transfer? | 14:43 |
| dansmith | sure | 14:43 |
| dansmith | but with the reasoning to reject the alternative as incorrectly linking weird glance behavior with the transfer mechanics of the client as considered harmful :D | 14:44 |
| abhishekk | :d | 14:44 |
| abhishekk | thank you, I will update the spec by tomorrow eod | 14:44 |
| croelandt | good, anything else for today? | 14:45 |
| dansmith | thanks | 14:45 |
| mhen | hi! regarding the image encryption patchset ... | 14:45 |
| mhen | still on it - need to look into the image import stuff a bit more to allow glance-direct and copy-image to be used on encrypted images, which the current state of the patchset does not allow but I agree would be useful | 14:45 |
| abhishekk | no, please look at scrubber patch | 14:45 |
| mhen | I'm currently involved in a project for the next few weeks that limits my time to work on this even more than usual unfortunately though :/ | 14:45 |
| abhishekk | mhen: let me know if you need anything | 14:45 |
| mhen | thanks, will do | 14:46 |
| croelandt | yeah don't hesitate to ping us directly | 14:46 |
| abhishekk | thanks for the update, as promised during PTG I will or someone from team will look at tempest coverage for this | 14:46 |
| croelandt | Anything else? | 14:47 |
| abhishekk | dansmith: I guess we need to specs for this topic now since API side change is also involved :o | 14:48 |
| abhishekk | to --- two | 14:48 |
| dansmith | it's already a spec? | 14:48 |
| dansmith | or you mean instead of spec-lite? | 14:48 |
| dansmith | oh two specs.. do we? | 14:48 |
| dansmith | in nova, it'd be one, I'm fine with one for sure :) | 14:48 |
| abhishekk | for glanceclient I can convert it to lite spec and main spec will be in glance | 14:48 |
| abhishekk | ack, then lets go wit one spec only :D | 14:49 |
| abhishekk | @croelandt nothing else :D | 14:49 |
| dansmith | oh because there are separate dirs.. well, whatever, I think one should be plenty but I defer to croelandt I guess :) | 14:49 |
| abhishekk | I will move the spec to glance instead of client | 14:50 |
| croelandt | good | 14:51 |
| croelandt | I guess we have no additional topics, so thanks everyone for joining! | 14:51 |
| abhishekk | thank you!! | 14:51 |
| croelandt | #endmeeting | 14:52 |
| opendevmeet | Meeting ended Thu Apr 24 14:52:13 2025 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) | 14:52 |
| opendevmeet | Minutes: https://meetings.opendev.org/meetings/glance/2025/glance.2025-04-24-14.00.html | 14:52 |
| opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/glance/2025/glance.2025-04-24-14.00.txt | 14:52 |
| opendevmeet | Log: https://meetings.opendev.org/meetings/glance/2025/glance.2025-04-24-14.00.log.html | 14:52 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!