| opendevreview | Matthew Oliver proposed openstack/swift master: sharder: update shard storage_policy_index if roots changes https://review.opendev.org/c/openstack/swift/+/800748 | 05:37 |
|---|---|---|
| mattoliver | ^ that just reverts back to patchset 13, moves it after the shard spi listing patch so we can improve it's probe test. | 05:38 |
| opendevreview | Matthew Oliver proposed openstack/swift master: trace: move to a RequestTraceMiddleware https://review.opendev.org/c/openstack/swift/+/803879 | 07:28 |
| clayg | acoles: so i was really hoping after @tburke and I WTF'd all over the proxy/controller/container change in p 803423 you'd come back with something like "oh yeah, this is a bit wonky, I thought about doing <this way more obviously correct thing> so maybe I'll give that go" | 14:32 |
| clayg | but it sounds like you an Matt were like "yeah, it's complicated, that's what the tests are for: get over it" | 14:33 |
| acoles | I'm not sure I think it is complicated | 14:33 |
| acoles | my (mis?) reading of the comments was that it has become apparent that the headers were additive | 14:34 |
| clayg | acoles: well one thing that's surprising/confusing is the "if policy_key in resp.headers" - that part only seems to be True if 1) the resp wasn't constructed from container_info and 2) we're talking to an old server | 14:36 |
| clayg | the other thing that's surprising/confusing is the "and not in req.headers" - since "we" never set req.headers['x-backend-spi'] it's not obvious why it would ever be false, it's not false when a client request is going to a root - but it does seem after some recursion it *can* get set on "the" req (when "the" req is a sub-shard request) | 14:38 |
| clayg | ... but then i'm seeing "oh we're not setting it" except "oh if we don't set it we still use req.headers" | 14:39 |
| clayg | so then I'm thinking "both the LHS and RHS of this conditional are surprising/confusing" - which leads me to: don't we ALWAYS have this info in container_info? why is ANY of this conditional at all - we KNOW what we want the value to be | 14:39 |
| clayg | tburke points out we don't "have" to use the cahced container info if we have a listing response from an upgraded root that contains the x-backend-spi - but that seems like a minor imporovement - and it could still be non-conditional | 14:40 |
| clayg | which was why I was hoping you'd you have brilliant plan | 14:41 |
| acoles | but we do set req.headers['x-backend-spi'] - that's what the patch does | 14:41 |
| clayg | no we set extra_headers[x-backend-spi] 🤷♂️ | 14:42 |
| acoles | and once it is set we need to check if it is in the request, and not override it with the wrong value | 14:42 |
| acoles | extra_headers are then set in a subrequest which is then handled by the same code | 14:42 |
| acoles | so the subrequest becomes req | 14:43 |
| acoles | and we don't necessarily have the correct spi in container_info when the method recurses | 14:43 |
| clayg | i'm not sure if you're arguing "look, this code works" (I agree) or "you are wrong, this is about as good as we can hope for and it's not even that confusing" (you might be right, but i'm not willing to concede so easily) | 14:44 |
| acoles | I think it is wrong to say that we have the value in container_info, I think we need to set the header on the subrequests | 14:45 |
| clayg | yeah the recursion I guess is what i'm worried might be harder than i'm realizing, sometimes when I see a method foo() call it self it uses a foo(ctx=ctx) thing to pass state forward - perhaps no such plumbing is available or could be added here | 14:45 |
| acoles | we send a whole new subrequest into the proxy app | 14:46 |
| clayg | i also think we need to set the header on the subrequests - which is why it looks weird to see us have extra_headers['x-backend-spi'] as *a multi-part and complex conditional* | 14:46 |
| acoles | ok, take out the 'if policy_key in resp.headers' part - are we guaranteed it is always there, old servers etc? if so then we can lose it. It just seemed sensibly defensive. | 14:47 |
| clayg | like there's two parts 1) where (or maybe more accurately *when*) do we get the value and 2) how do we make sure we set it on ALL the remaining subrequests - I think this conditional is mixing the two and confusing me and maybe tim | 14:48 |
| clayg | acoles: it seemed to me when talking to an old server we did not have that header - but it would be there *currently* on any "resp" built form cache - this was not obvious - this is related to part #1 "when and where do we get the value" | 14:49 |
| clayg | like if we can currently get it (from container_info) onto a resp built from the shard_range body from cache - we could probably annotate un-cached resp from old servers too? | 14:51 |
| clayg | but maybe not, maybe sometimes we have to be willing to move forward without "knowing" the spi of the root - and that might *still* fall into part #1 (when and where do we get THE value) | 14:51 |
| clayg | if we decide the best we can do for THE value is None - we don't want to suddenly half-way through decide THE value is some subshard resp backend-spi and propogating that? or maybe we do... i kind of stopped thinking about it at that point | 14:53 |
| clayg | so... there's no obvious way to spell "this is the shard-range resp from the ROOT" in _get_from_shards or even GETorHEAD 🤔 i guess I could inspect self.account_name 😁 | 15:03 |
| opendevreview | Clay Gerrard proposed openstack/swift master: DNM: set and forget root spi https://review.opendev.org/c/openstack/swift/+/804106 | 15:56 |
| clayg | acoles: ^ take a look! you might say "yeah... MAYBE" or it could be obviously broken and I still have some work todo to understand the mechanism we have to forward these headers (the part #2 - after we figure out the root spi; how to do we ensure it's propogated) | 16:54 |
| acoles | clayg: ack, I'll take a look tomorrow | 17:05 |
| timburke | acoles, fwiw i did some digging -- looks like x-backend-storage-policy-index should always be in the container-server GET response (at least, for any swift since storage policies were introduced ;-) | 18:21 |
| timburke | well, any *successful* GET response | 18:22 |
| zaitcev | Guys | 22:19 |
| zaitcev | Yet another shameful admission by a core reviewer | 22:19 |
| zaitcev | I cannot figure out how get_more_nodes works. | 22:20 |
| zaitcev | Or, rather, the code makes sense, but I am trying to understand what determines the maximum number of handoffs. | 22:20 |
| zaitcev | Looks like it can return an absurd amount on a system with large number of zones. | 22:21 |
| zaitcev | Can be thousands easily. | 22:26 |
| zaitcev | So, every user of get_more_nodes() limits the number of nodes. Like handoff_nodes = itertools.islice(handoff_nodes, len(primary_nodes)) | 22:27 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!