| rosmaita | dansmith: around? | 13:12 |
|---|---|---|
| dansmith | yup | 13:26 |
| dansmith | rosmaita: sorry I see I missed a ping in a meeting just now | 13:30 |
| rosmaita | dansmith: np, Luzi had a question, but she put it on the agenda for the glance meeting on thursday | 13:31 |
| dansmith | rosmaita: okay, can we discuss in either of the reviews? | 13:31 |
| rosmaita | dansmith: probably, here's what the issue is: https://meetings.opendev.org/meetings/image_encryption/2021/image_encryption.2021-06-21-13.00.log.html#l-21 | 13:34 |
| dansmith | okay where's the non-lite spec? | 13:36 |
| rosmaita | https://specs.openstack.org/openstack/glance-specs/specs/victoria/approved/glance/image-encryption.html | 13:36 |
| dansmith | right, okay, but this lite spec is specifically about the interaction between glance and barbican right? | 13:38 |
| dansmith | my comments about what to do in the error path relate more specifically to that part itself | 13:38 |
| rosmaita | well, the spec-lite is to decouple the barbican part from the glance part | 13:38 |
| rosmaita | and go ahead and do the glance part first | 13:39 |
| dansmith | well, right, but that's where we get into the problem of allowing it to be so decoupled that we may (as the user expects) or may not have told barbican that a key is in use | 13:40 |
| rosmaita | yeah, the spec-lite is basically how to do this without having the ability to tell barbican a key is in use | 13:40 |
| rosmaita | because even with a key registered as in use, the owner can still delete the key | 13:41 |
| *** abhishekk is now known as akekane|home | 13:41 | |
| rosmaita | so i think getting the error conditions worked out is important | 13:41 |
| dansmith | sure, but at least you're accounting for them so they're deleting "against medical advice" right | 13:41 |
| rosmaita | (as you pointed out) | 13:41 |
| rosmaita | right, so the medical advice part will be an enhancement | 13:41 |
| dansmith | yeah, but the user won't be able to tell whether they have a glance/barbican configuration that does the accounting or not right? | 13:42 |
| rosmaita | well, they should assume no accounting | 13:43 |
| dansmith | you mean assume until it's implemented, right? | 13:43 |
| rosmaita | yes, the problem is there isn't an ETA for implementation | 13:44 |
| dansmith | yeah, I understand | 13:44 |
| rosmaita | and i would really like to see tempest tests for this, because i imagine there are some design holes that haven't been foreseen yet | 13:44 |
| dansmith | just thinking that in two years when there is, glance uses it, people get used to the fact that it happens, and then don't realize that "cluster 127 hasn't been updated, I didn't realize, so I thought I was deleting an unused key" | 13:44 |
| dansmith | so if there's some flag we can design in to "delete against medical advice" and always require it now, because we can't guarantee the accounting, or something like that | 13:45 |
| dansmith | that would be safer and more future proof | 13:45 |
| rosmaita | well, we could do what cinder does | 13:45 |
| dansmith | or "create this image and don't obsess over the accounting" | 13:46 |
| rosmaita | https://specs.openstack.org/openstack/glance-specs/specs/train/approved/glance/barbican-secret-deletion-support.html | 13:47 |
| rosmaita | except the deletion policy would be "false" by default | 13:48 |
| rosmaita | doesn't solve the problem of end users deleting in-use secrets, though | 13:48 |
| dansmith | I guess it seems like glance should try to be internally consistent, so if you take the create image case as an example: | 13:50 |
| dansmith | The user is asking glance to create an image which uses an encryption key.. there are two behaviors you could assert here: | 13:50 |
| dansmith | 1. Glance should not create (or leave) the image if it can't register itself as a consumer | 13:51 |
| dansmith | 2. Glance should ignore a register_as_consumer() failure and prioritize the image creation | 13:51 |
| dansmith | if you let the user opt into either behavior, then we can do #2 today, and reject all attempts for #1 since we can't do that thing | 13:52 |
| dansmith | then in two years, #2 is still valid, and #1 becomes valid once we can | 13:52 |
| dansmith | and you can make #2 the default if you don't provide the onfail=delete flag or whatever | 13:52 |
| rosmaita | that seems reasonable | 13:52 |
| *** akekane|home is now known as abhishekk | 13:55 | |
| abhishekk | rosmaita, jokke_, kindly go through policy refactoring spec as well | 13:58 |
| abhishekk | https://review.opendev.org/c/openstack/glance-specs/+/796753 | 13:58 |
| rosmaita | abhishekk: ack | 13:59 |
| abhishekk | thanks | 13:59 |
| abhishekk | dansmith, I have one question related to quota | 14:02 |
| abhishekk | after implementing the usage API | 14:02 |
| abhishekk | can we restrict the import operation if it is committing over quota? | 14:03 |
| dansmith | abhishekk: you mean the actual import call after stage? | 14:04 |
| abhishekk | yes | 14:04 |
| dansmith | my code should already do that, because it now sets image.size when you stage, so import can consider that | 14:04 |
| dansmith | but I wrote it so long ago, I can barely remember :/ | 14:04 |
| abhishekk | ack, I can understand :D | 14:05 |
| dansmith | abhishekk: https://review.opendev.org/c/openstack/glance/+/788077/15/glance/async_/flows/api_image_import.py line 875 | 14:07 |
| dansmith | but I'm only doing that for copy_image I guess, | 14:08 |
| dansmith | so we should be able to open that to all of them I think, because of: https://review.opendev.org/c/openstack/glance/+/788075/11 | 14:08 |
| abhishekk | yeah | 14:09 |
| dansmith | er, that's enforcing stage_total | 14:10 |
| abhishekk | also this is different check | 14:10 |
| abhishekk | yeah | 14:10 |
| dansmith | but same difference for size on glance-direct | 14:10 |
| abhishekk | image_size_total | 14:10 |
| abhishekk | What I am saying is in current implementation | 14:11 |
| abhishekk | 1. suppose image_size_total is 10 GB | 14:11 |
| abhishekk | 2. We have 9 GB filled | 14:11 |
| abhishekk | 3. Image ins stage is of 2 GB | 14:12 |
| abhishekk | IN this case current import operation will allow this import call and next will be rejected | 14:12 |
| abhishekk | Now after your usage API call, at point 3 is it possible to reject call there only? | 14:12 |
| dansmith | well, we can't really reject the api call because get_flow() happens async (which was a problem for the locking before you know) | 14:14 |
| dansmith | but we can prevent the actual import yes | 14:14 |
| abhishekk | ack, just wanted to know the possibility | 14:15 |
| dansmith | yup, this is a good catch | 14:15 |
| dansmith | there are far too many ways to create an image in glance :) | 14:16 |
| abhishekk | :D | 14:17 |
| abhishekk | thank you rosmaita for review | 14:25 |
| rosmaita | np | 14:26 |
| dansmith | abhishekk: okay so looking through the tests, I remember now.. We do stop import at the api layer once you're over your size_quota, but not if you will go over it with the next operation | 14:38 |
| dansmith | making it behave more like upload | 14:38 |
| dansmith | but I think that since we know the size at that point, we might as well do what you describe.. the unfortunate part is that right now our check returns a 413 to the user immediately, | 14:39 |
| dansmith | and afterwards they will get a 20x and then have to poll the task | 14:39 |
| dansmith | lemme get you a link | 14:39 |
| dansmith | https://review.opendev.org/c/openstack/glance/+/788055/14/glance/tests/functional/v2/test_images.py L7097 | 14:40 |
| abhishekk | looking | 14:40 |
| dansmith | we should still keep that check so that if you went over your size for some other reason, we still get the 413 if we know you're already over | 14:40 |
| dansmith | so I will add another scenario to those tests to trigger that as well | 14:40 |
| abhishekk | sounds good | 14:42 |
| dansmith | abhishekk: sorry that took longer than expected because I had to move some things up in the stack.. here comes: | 15:46 |
| abhishekk | no problem at all | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Refactor SynchronousAPIBase for more cases https://review.opendev.org/c/openstack/glance/+/788065 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Update image.size after conversion https://review.opendev.org/c/openstack/glance/+/788091 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Make image stage set image.size https://review.opendev.org/c/openstack/glance/+/788075 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Drop lower-constraints jobs https://review.opendev.org/c/openstack/glance/+/782768 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add unified quotas infrastructure https://review.opendev.org/c/openstack/glance/+/788054 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Enforce keystone limits for image upload https://review.opendev.org/c/openstack/glance/+/788055 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_staging_usage() to DB API https://review.opendev.org/c/openstack/glance/+/788076 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_stage_total quota enforcement https://review.opendev.org/c/openstack/glance/+/788077 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_image_count() to DB API https://review.opendev.org/c/openstack/glance/+/788326 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_count_total quota enforcement https://review.opendev.org/c/openstack/glance/+/788327 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_uploading_count() to DB API https://review.opendev.org/c/openstack/glance/+/794245 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_count_uploading quota enforcement https://review.opendev.org/c/openstack/glance/+/794247 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: Add unified quotas documentation https://review.opendev.org/c/openstack/glance/+/794672 | 15:47 |
| opendevreview | Dan Smith proposed openstack/glance master: ULTRAWIP: Example usage API https://review.opendev.org/c/openstack/glance/+/794860 | 15:47 |
| abhishekk | Just making sure the chain is right, I can see merged patch on the top of all patches | 15:54 |
| dansmith | probably just because I rebased on master | 15:57 |
| abhishekk | ok | 16:00 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/788055/14..15/glance/async_/flows/api_image_import.py | 16:00 |
| abhishekk | line #753 | 16:00 |
| abhishekk | any specific reason to restore image status to queued and not uploading | 16:01 |
| dansmith | domain proxy won't let us go back to uploading :( | 16:01 |
| dansmith | see the comment in the test I added for that | 16:01 |
| abhishekk | It will be confusing to have image with status queued and size also set. | 16:01 |
| dansmith | https://review.opendev.org/c/openstack/glance/+/788055/14..15/glance/tests/functional/v2/test_images.py | 16:02 |
| dansmith | L7130 | 16:02 |
| dansmith | I agree, and it's also wasteful to require re-staging | 16:02 |
| dansmith | this is similar to the policy check problems, where the business logic is embedded deep in the stack, such that even import can't revert a state | 16:03 |
| abhishekk | I think in staging we have a check to check file size and restage if it does not match | 16:03 |
| dansmith | the API can enforce state transition restrictions, but doing it so deep is problematic | 16:03 |
| abhishekk | agree, onion always makes you cry :D | 16:03 |
| dansmith | abhishekk: yikes, what if I stage a different 4.0000GiB image and you just take the old one? | 16:03 |
| abhishekk | hmm | 16:04 |
| abhishekk | and also that check is for copy-image import method only | 16:10 |
| abhishekk | I guess we can make the check global and use checksum verification as well (as an enhancement) | 16:10 |
| * abhishekk going for dinner break | 16:19 | |
| opendevreview | Dan Smith proposed openstack/glance master: Add unified quotas infrastructure https://review.opendev.org/c/openstack/glance/+/788054 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Enforce keystone limits for image upload https://review.opendev.org/c/openstack/glance/+/788055 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_staging_usage() to DB API https://review.opendev.org/c/openstack/glance/+/788076 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_stage_total quota enforcement https://review.opendev.org/c/openstack/glance/+/788077 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_image_count() to DB API https://review.opendev.org/c/openstack/glance/+/788326 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_count_total quota enforcement https://review.opendev.org/c/openstack/glance/+/788327 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add user_get_uploading_count() to DB API https://review.opendev.org/c/openstack/glance/+/794245 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add image_count_uploading quota enforcement https://review.opendev.org/c/openstack/glance/+/794247 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: Add unified quotas documentation https://review.opendev.org/c/openstack/glance/+/794672 | 17:57 |
| opendevreview | Dan Smith proposed openstack/glance master: ULTRAWIP: Example usage API https://review.opendev.org/c/openstack/glance/+/794860 | 17:57 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!