Tuesday, 2021-08-10

opendevreviewMatthew Oliver proposed openstack/swift master: sharder: update shard storage_policy_index if roots changes  https://review.opendev.org/c/openstack/swift/+/80074805: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
opendevreviewMatthew Oliver proposed openstack/swift master: trace: move to a RequestTraceMiddleware  https://review.opendev.org/c/openstack/swift/+/80387907:28
claygacoles: 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
claygbut it sounds like you an Matt were like "yeah, it's complicated, that's what the tests are for: get over it"14:33
acolesI'm not sure I think it is complicated14:33
acolesmy (mis?) reading of the comments was that it has become apparent that the headers were additive14:34
claygacoles: 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 server14:36
claygthe 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
claygso 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 be14:39
claygtburke 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-conditional14:40
claygwhich was why I was hoping you'd you have brilliant plan14:41
acolesbut we do set req.headers['x-backend-spi']  - that's what the patch does14:41
claygno we set extra_headers[x-backend-spi] 🤷‍♂️14:42
acolesand once it is set we need to check if it is in the request, and not override it with the wrong value14:42
acolesextra_headers are then set in a subrequest which is then handled by the same code14:42
acolesso the subrequest becomes req14:43
acolesand we don't necessarily have the correct spi in container_info when the method recurses 14:43
claygi'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
acolesI think it is wrong to say that we have the value in container_info, I think we need to set the header on the subrequests14:45
claygyeah 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 here14:45
acoleswe send a whole new subrequest into the proxy app14:46
claygi 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
acolesok, 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
clayglike 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 tim14:48
claygacoles: 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
clayglike 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
claygbut 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
claygif 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 point14:53
claygso... 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
opendevreviewClay Gerrard proposed openstack/swift master: DNM: set and forget root spi  https://review.opendev.org/c/openstack/swift/+/80410615:56
claygacoles: ^ 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
acolesclayg: ack, I'll take a look tomorrow17:05
timburkeacoles, 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
timburkewell, any *successful* GET response18:22
zaitcevGuys22:19
zaitcevYet another shameful admission by a core reviewer22:19
zaitcevI cannot figure out how get_more_nodes works.22:20
zaitcevOr, rather, the code makes sense, but I am trying to understand what determines the maximum number of handoffs.22:20
zaitcevLooks like it can return an absurd amount on a system with large number of zones.22:21
zaitcevCan be thousands easily.22:26
zaitcevSo, 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/!