| opendevreview | Eric Xie proposed openstack/glance master: Remove single quotes in excetion message https://review.opendev.org/c/openstack/glance/+/807827 | 06:40 |
|---|---|---|
| opendevreview | Pranali Deore proposed openstack/glance-tempest-plugin master: Add protection testing for metadef namespaces https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902 | 08:47 |
| opendevreview | Pranali Deore proposed openstack/glance-tempest-plugin master: Add protection testing for metadef namespaces https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902 | 10:29 |
| *** tosky is now known as Guest6702 | 15:22 | |
| *** tosky_ is now known as tosky | 15:22 | |
| opendevreview | Dan Smith proposed openstack/glance master: [uwsgi] Add missing pefetch periodic job https://review.opendev.org/c/openstack/glance/+/803937 | 16:02 |
| abhishekk | sorry I was not around today | 16:54 |
| dansmith | abhishekk: I pushed up that prefetcher thing | 16:57 |
| dansmith | not sure if we have any real tests for that or not | 16:57 |
| abhishekk | yep, looking at that | 16:57 |
| dansmith | I added some that actually run it enough I think, but.. | 16:57 |
| abhishekk | No we don't have "real" functional test for it | 16:57 |
| dansmith | okay | 16:57 |
| abhishekk | just one thing, should we pass authorization_layer=false to get_image_factory and get_repo ? | 17:01 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/803937/2/glance/tests/unit/test_image_cache.py#538 | 17:01 |
| dansmith | oh I guess, but doesn't really matter I don't think | 17:01 |
| opendevreview | Dan Smith proposed openstack/glance master: [uwsgi] Add missing pefetch periodic job https://review.opendev.org/c/openstack/glance/+/803937 | 17:02 |
| abhishekk | super fast :D | 17:03 |
| dansmith | meh :) | 17:04 |
| dansmith | abhishekk: anyway, not saying we should merge this still this cycle, just wanted to get it done | 17:04 |
| abhishekk | dansmith, ack, I need to run some manual operations on it, but overall it looks good to me | 17:04 |
| dansmith | cool, that would be good to test it for real for sure | 17:05 |
| abhishekk | and I think new unit tests looks solid as well | 17:05 |
| dansmith | you know, | 17:05 |
| dansmith | if we added a new thing to the cache api, we could force a synchronous cache run and maybe test this from tempest | 17:05 |
| dansmith | some people might like that too | 17:06 |
| abhishekk | good idea | 17:06 |
| dansmith | I don't really mean synchronous, but.. immediate | 17:06 |
| abhishekk | yep, i think that will make this cache work full proof as well | 17:07 |
| dansmith | certainly some people will want to queue an image and then say "okay now go cache it, don't wait an hour" | 17:07 |
| abhishekk | I have opted for that earlier, but we definitely discuss this during Yoga PTG | 17:08 |
| dansmith | ack | 17:09 |
| dansmith | nova's cache api is more directed, FWIW | 17:09 |
| abhishekk | ok | 17:09 |
| dansmith | you don't really queue an image and have it maintain the image in the cache when it runs the next time | 17:09 |
| dansmith | you say "cache this image now" and it tells the computes to do it now, then if you don't re-request it often enough, it will age out of the cache like any other image we would cache as a result of a boot | 17:10 |
| dansmith | it's a little different use-case for glance, but it's much less complicated too :) | 17:10 |
| dansmith | er, pva | 17:10 |
| dansmith | er, nova's api I mean.. there's no extra bookkeeping | 17:11 |
| abhishekk | right | 17:11 |
| abhishekk | the glance cache workflow also includes some incomplete, pending operations I guess which are not required any more IMO | 17:12 |
| abhishekk | incomplete and invalid cache operations those are ambiguous IMO | 17:13 |
| dansmith | presumably the former means "in progress" and the latter means "I couldn't do that" ? | 17:14 |
| dansmith | but from the api, I thought there was only queued and cached... | 17:14 |
| abhishekk | right | 17:14 |
| abhishekk | similar to prefetcher we also need to move cache-cleaner and cache-pruner under API as periodic jobs | 17:15 |
| dansmith | oh, those are separate/ | 17:16 |
| abhishekk | yes :D | 17:16 |
| abhishekk | and probably broken as well | 17:16 |
| dansmith | heh | 17:17 |
| dansmith | hmm, so if you don't run the cache cleaner your cache will eventually just fill up? | 17:17 |
| abhishekk | yes | 17:18 |
| dansmith | sweet! | 17:18 |
| dansmith | well, in that case, hope it works :) | 17:19 |
| abhishekk | unless you delete those using glance-cache-manage | 17:19 |
| dansmith | abhishekk: isn't this an api behavioral change? https://review.opendev.org/c/openstack/glance/+/804966 | 17:25 |
| abhishekk | looking | 17:25 |
| dansmith | someone with current code that thinks they can create/delete tags by just passing the desired set will now find they're not actually removing tags | 17:27 |
| abhishekk | I think we are setting image status to killed if upload fails | 17:27 |
| dansmith | I definitely agree that the API should have been that way in the past, but since it wasn't (and we don't have microversions!) this would just be a breaking change | 17:28 |
| abhishekk | oops this is about tags | 17:28 |
| dansmith | yeah | 17:29 |
| abhishekk | I think this is by mistake and not documented anywhere as well | 17:29 |
| dansmith | abhishekk: it's documented by virtue of the current behavior | 17:29 |
| abhishekk | that it will erase existing set of tags and replaces it with new | 17:29 |
| dansmith | right, but in the absence of documentation, the behavior is what people will code to :) | 17:30 |
| abhishekk | ack | 17:30 |
| abhishekk | frankly, I was not familiar with metadef before and really don't know the correct behavior in this case | 17:31 |
| abhishekk | just found that if you create single tag then it will not replace the old one, but if you create multiple at a time it will erase all existing tags | 17:32 |
| dansmith | well, correct behavior or not, the way it behaves now is what people may be relying on | 17:32 |
| abhishekk | hmm | 17:32 |
| dansmith | "If an API’s behavior isn’t adequately documented, then developers using the API have no choice but to go by what they observe the behavior to be. A change that will violate those observations is a change that requires a version." | 17:33 |
| dansmith | from: https://specs.openstack.org/openstack/api-wg/guidelines/api_interoperability.html#interoperability | 17:33 |
| dansmith | (examples section, example of "it's not documented") | 17:33 |
| dansmith | now, I know that glance doesn't have microversions to allow a change like this, but I think that means you opt to not change it unless it's security or correctness related | 17:34 |
| abhishekk | ack, thanks then in that case it is a behavioral change | 17:34 |
| dansmith | okay I can comment | 17:34 |
| abhishekk | So, if we want to correct it then, we need to deprecate this API and add new one ? | 17:35 |
| dansmith | well, one way to handle this is to add a parameter with the default that matches the current behavior | 17:35 |
| abhishekk | or let it be as it is until someone complains | 17:35 |
| dansmith | so ?append=False | 17:35 |
| dansmith | pass ?append=True when you want to append to the current set of tags | 17:35 |
| abhishekk | yep, that sounds better | 17:36 |
| dansmith | you can make the glanceclient take the new behavior, that's no problem | 17:36 |
| dansmith | but the REST API should not change unless asked | 17:36 |
| abhishekk | I liked this idea | 17:36 |
| abhishekk | positive scenarios are working as expected with recent cache-prefetcher patch | 17:40 |
| dansmith | on the killed thing, | 17:40 |
| dansmith | I think we only set that if we fail the signature check, | 17:40 |
| abhishekk | right | 17:40 |
| dansmith | which is probably not what we want anyway (maybe cruft?) | 17:40 |
| dansmith | everything else goes back to queued | 17:40 |
| dansmith | presumably based on the bug, once it goes to killed it's just invisible to the uploader? | 17:41 |
| dansmith | or maybe they can still GET it but not list? | 17:41 |
| abhishekk | I think it will br invisible to uploader | 17:42 |
| dansmith | so that's broken and we should make it go back to queued right? | 17:42 |
| dansmith | https://docs.openstack.org/glance/wallaby/user/statuses.html | 17:42 |
| dansmith | this ^ shows that only happens with v1 | 17:42 |
| dansmith | so right now they can't even see that it goes to killed state, which means it just looks like a bug to them and not even a violation of the docs | 17:43 |
| abhishekk | yep, I think this should be set back to queued | 17:45 |
| opendevreview | Dan Smith proposed openstack/glance master: Make signature verification go back to 'queued' https://review.opendev.org/c/openstack/glance/+/807932 | 17:49 |
| dansmith | there was actually a unit test (!), running functional now | 17:50 |
| abhishekk | also I can see some tests where we are setting image status to kille if location add/update fails | 17:50 |
| dansmith | do you want a separate bug for just the "image should not go to killed" thing or is the related-bug enough? | 17:50 |
| abhishekk | related is enough | 17:50 |
| dansmith | \o/ | 17:51 |
| abhishekk | test_location_add_not_permitted_status_killed | 17:51 |
| abhishekk | So I think this test also needs to be removed now | 17:53 |
| abhishekk | test_location_replace_not_permitted_status_killed | 17:53 |
| dansmith | is it checking for killed specifically in the api or just checking against a set of valid states? | 17:53 |
| abhishekk | API | 17:54 |
| dansmith | it's checking (active, queued) | 17:54 |
| dansmith | so the test isn't really wrong, but it might not be possible in real life anymore | 17:55 |
| dansmith | so I dunno that it needs to be removed really, but whatever you want :) | 17:55 |
| abhishekk | ack | 17:56 |
| abhishekk | I think let it be there, I will have some one to work on look at 'killed' presence in v2 API and clean it at once | 17:57 |
| abhishekk | so again, may be I am silly but the API WG thing should be applicable here or not ? | 17:59 |
| dansmith | you could argue it for sure, but that's why I asked if the user can see that it went to killed | 18:00 |
| dansmith | if they can, then yes, probably | 18:00 |
| dansmith | but if they can't then it's just disappearing | 18:00 |
| abhishekk | Its definitely not in the list | 18:01 |
| dansmith | but also, the docs clearly show it goes to queued for v2, and does for every other situation | 18:01 |
| dansmith | in the tags case, the behavior change will result in the wrong thing happening (i.e. tags being left on the image that the caller intended to remove) | 18:01 |
| dansmith | in this case, I don't think you could argue that having the image disappear and leak in the database is what they expected based on previous behavior :) | 18:02 |
| abhishekk | ++ | 18:02 |
| dansmith | also, | 18:03 |
| dansmith | the guidelines say that versions are not needed to keep wrong behavior | 18:03 |
| dansmith | meaning if something is currently returning 500, you don't need to continue to return 500 on old microversions, because that's clearly error behavior | 18:03 |
| abhishekk | yep, makes sense | 18:04 |
| * abhishekk signing out for the day | 18:54 | |
| *** timburke__ is now known as timburke | 20:04 | |
| opendevreview | Merged openstack/glance master: Add missing parameters for the healthcheck middleware https://review.opendev.org/c/openstack/glance/+/804563 | 20:39 |
| opendevreview | Merged openstack/glance master: Add missing [oslo_reports] options https://review.opendev.org/c/openstack/glance/+/804569 | 20:39 |
| opendevreview | Dan Smith proposed openstack/glance master: Reproduce bug #1932337 https://review.opendev.org/c/openstack/glance/+/796885 | 20:51 |
| opendevreview | Dan Smith proposed openstack/glance master: Fix failed cinder store migration for non-owners https://review.opendev.org/c/openstack/glance/+/796887 | 20:51 |
| -opendevstatus- NOTICE: The Gerrit service on review.opendev.org is going offline momentarily for a host migration and zuul upgrade, downtime should be only a few minutes. | 21:05 | |
| opendevreview | Dan Smith proposed openstack/glance master: Reproduce bug #1932337 https://review.opendev.org/c/openstack/glance/+/796885 | 21:21 |
| opendevreview | Dan Smith proposed openstack/glance master: Fix failed cinder store migration for non-owners https://review.opendev.org/c/openstack/glance/+/796887 | 21:21 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!