Monday, 2021-03-22

*** rcernin has joined #openstack-lbaas00:14
*** rcernin has quit IRC00:22
*** rcernin has joined #openstack-lbaas00:22
*** ramishra has quit IRC02:17
*** sapd1 has joined #openstack-lbaas02:19
*** psachin has joined #openstack-lbaas03:22
*** rcernin has quit IRC03:22
*** rcernin has joined #openstack-lbaas04:02
*** ramishra has joined #openstack-lbaas04:02
*** rcernin has quit IRC04:07
*** rcernin has joined #openstack-lbaas04:48
*** rcernin has quit IRC04:48
*** sapd1 has quit IRC04:53
*** vishalmanchanda has joined #openstack-lbaas05:11
*** rcernin has joined #openstack-lbaas05:26
*** ccamposr has joined #openstack-lbaas07:35
*** rpittau|afk is now known as rpittau08:19
*** rcernin has quit IRC08:23
*** sapd1 has joined #openstack-lbaas08:29
*** ramishra has quit IRC08:31
*** ramishra has joined #openstack-lbaas08:36
*** rcernin has joined #openstack-lbaas08:58
*** sapd1 has quit IRC09:12
openstackgerritGregory Thiemonge proposed openstack/octavia-tempest-plugin master: DNM test adding delays between least_connection requests
*** rcernin has quit IRC09:37
*** ataraday has quit IRC09:44
*** sapd1 has joined #openstack-lbaas10:04
*** rcernin has joined #openstack-lbaas11:13
*** rcernin has quit IRC11:18
*** sapd1 has quit IRC11:38
*** rcernin has joined #openstack-lbaas11:44
*** servagem has joined #openstack-lbaas11:59
*** rcernin has quit IRC12:31
*** rcernin has joined #openstack-lbaas12:31
*** jamesdenton has quit IRC12:48
*** jamesdenton has joined #openstack-lbaas12:49
*** psachin has quit IRC13:04
openstackgerritBodo Petermann proposed openstack/octavia master: Fix LB failover for amphorav2: set security group
*** rcernin has quit IRC14:05
*** luksky has joined #openstack-lbaas14:16
openstackgerritGregory Thiemonge proposed openstack/octavia-tempest-plugin master: DNM test adding delays between least_connection requests
*** sapd1 has joined #openstack-lbaas14:31
*** __ministry1 has joined #openstack-lbaas15:23
openstackgerritGregory Thiemonge proposed openstack/octavia master: Fix using subnets with host_routes in amphorav2 driver
*** __ministry1 has quit IRC15:40
*** xgerman has joined #openstack-lbaas16:10
*** vishalmanchanda has quit IRC16:11
*** rpittau is now known as rpittau|afk17:11
openstackgerritBodo Petermann proposed openstack/octavia master: Fix LB failover for amphorav2: set security group
*** jamesdenton has quit IRC17:24
*** jamesdenton has joined #openstack-lbaas17:24
*** sapd1 has quit IRC19:36
*** rcernin has joined #openstack-lbaas20:25
*** rcernin has quit IRC20:38
*** ccamposr has quit IRC21:00
*** ccamposr has joined #openstack-lbaas21:00
*** rcernin has joined #openstack-lbaas21:03
*** rcernin has quit IRC21:17
rm_workjohnsom: was this related to the issue before? recall we couldn't backport that21:48
johnsomFrom a quick read of the commit message there, no they are not related.21:48
rm_workfnpanic / johnsom: yeah, that multi-az patch was really good to merge I think at the time, but needs rebasing pretty heavily now. it would work fine as an "experimental" (non-default) feature21:48
johnsomYeah, that patch is already there and it is failing under this new scenario21:49
rm_workah hmm k21:49
johnsomrm_work Do you have time to look at that? I'm grooming the priority review list at the moment and planning to start working down it.21:50
fnpanicthanks for the update21:50
johnsomIt look like we have ~15 open bug patches21:51
rm_workmay have time this week21:51
*** rcernin has joined #openstack-lbaas21:51
*** rcernin has quit IRC21:51
johnsom+1 I also have a fix I need to make on Designate21:52
*** rcernin has joined #openstack-lbaas21:52
rm_worki also kinda want to pick up again <_<22:01
johnsomHa, yeah, that just drives me a bit nuts22:02
rm_workyeah such dumb cruft from old n-lbaas API22:07
rm_workwas that because we were considering allowing members to exist on multiple pools?22:08
rm_worklike to prevent multi-healthchecks or something?22:08
johnsomWell, the current model stops them from being shared IMO22:09
rm_workbut was that our excuse back then?22:31
johnsomv2 api design pre-dated me, just barely22:32
rm_workah, forgot that22:32
rm_workthought you were there from day 122:33
johnsomI think it was just modeling too close to the appliances, etc...22:33
johnsomI was for Octavia, but not v2 API22:33
rm_workI believe I was there for v2 as well... but didn't know enough to argue TOO much22:33
johnsomneutron-lbaas existed before I joined the party22:33
rm_workwell yeah, v1 predated me22:34
johnsomI think Mark was the only one really involved in v122:34
rm_workjohnsom: do you have that bug handy? I could look at it right now22:46
johnsomReally I think we just need to unlock the objects if we don't dispatch to the queue22:48
rm_workyeah hmm22:57
rm_workdeciding how to do that22:57
rm_workor whether to actually just dispatch to the queue rather than special-casing :D22:57
johnsomOr you could move the check back up to the API and just roll back the DB transaction22:58
johnsomNot sure if you need to be in the driver context to make that decision or not22:58
johnsomIt would be the inverse of
rm_workcould catch and rollback there23:00
rm_workjohnsom: are drivers *allowed* to raise exceptions besides those in the octavia_lib.api.drivers.exceptions file?23:07
rm_workthe selection there is ... slim23:07
rm_workI guess I could do it based on a *return*?23:08
johnsomProbably not23:08
rm_workor just hijack something like UnsupportedOption to mean "please rollback"23:08
johnsomI would really not do that....23:08
rm_workthen I can't get back up to the top-level to trigger a rollback23:09
rm_workunless I trigger a rollback IN THE DRIVER which seems gross23:09
johnsomNo, you would have to move the validation code up to the API23:09
rm_workthat's *all the logic* lol23:09
rm_worknot possible I think23:09
rm_workbut will look closer23:10
johnsomIt may not be possible. I haven't really looked at it. If not, it might just be easier to call a DB unlock method.23:10
johnsomNot that one that clean exists today, but....23:10
rm_workyeah if rollback is possible, that's definitely cleanest23:11
johnsomAh, I gave the wrong line before:
rm_workerr right i think i understood that23:12
rm_workanyway, i could add one more octavia-lib exception23:13
johnsomIt does look like a return value might pass up the driver chain. Maybe that is the right answer...23:13
rm_workNoAction or something23:13
rm_workyeah i will try that first, it should WORK23:13
rm_workit was more like "is that acceptable"23:13
rm_workyou wrote this driver model stuff mostly23:14
johnsomYeah, it is ... questionable23:14
johnsomI did23:14
rm_workfff I really really like all the new type-hint syntax in py3.???23:15
rm_workit's so good23:15
rm_worki wish I could use it all the time everywhere23:15
johnsomI haven't played with it much yet. Does python actually enforce it in some useful way?23:16
rm_workturns out I didn't actually want quite as loosely typed language as I thought23:16
johnsomOh, I am firmly in the "strong typed" came23:16
rm_workit'll definitely warn you... not sure if it enforces because I try not to violate it :D23:16
rm_workdef member_batch_update(self, pool_id: str, members: str) -> bool:23:17
rm_workdef member_batch_update(self, pool_id: str, members: list) -> bool:23:17
rm_workfeels pretty natural as far as extensions go23:18
rm_workI appreciate that it is optional per codebase23:18
rm_workor per def, really, but i'd enforce it i think if i was gonna use it at all23:18
johnsomI guess I would say adding an exception is probably the "right" answer. Like NoActionRequired or something makes sense.23:19
johnsomBut just looking at the API code I struggle to see how it can't figure out that it's a no-op call and just not call.23:19
rm_workyeah it MIGHT be possible to move it up23:20
rm_workwould be hilarious in that the driver function would just basically be a passthrough23:20
rm_worknot necessarily a bad thing?23:20
rm_workmight change the signature23:20
rm_workso ... not backportable tho23:21
rm_worktrying to also optimize for that23:21
johnsomPretty much what the driver should be really... Very light weight23:21
rm_workyeah so ... we can do that change in the future23:21
johnsom+1 on that backport-ability23:21
rm_workbut I'd like to make a fix that is easily backportable23:21
rm_workwhich might require you being willing to stretch what you're ok with23:21
rm_workIE, reusing an exception23:21
rm_workwhich is never used in another context in this part of the API23:22
rm_worklike `UnsupportedOptionError` :D23:22
rm_workthen I can follow-up with moving the logic entirely23:22
rm_workand we can backport just the first one23:22
johnsomDriverError seems closer to a bad choice, but not horrible23:23
rm_workwas looking for something slightly less generic23:23
johnsomYou could move the code out of the driver and still backport. This is all control plane side.23:23
rm_workthat's the base class23:23
rm_workmoving the code out of the driver would mean changing the signature23:24
rm_workunless ... I guess if you just want to run literally the same code twice23:24
rm_workit seems like there is no reason to not just pass the data that'd come out of the validation, straight to the cast()23:24
rm_workkinda lulz23:25
rm_workhmm i wonder if it's actually already doing 90% of this duplicated >_<23:26
johnsomYeah, I think so. There is marshaling games, but basically the same game, different formats.23:27
johnsomI just don't understand why you can check if they are all empty here:
johnsomLike it is doing here:
johnsomIf empty, rollback, if not dispatch23:28
rm_workyeah maybe i can23:28
rm_workdidn't realize it was already duplicating so much of this work23:28
rm_workwhich is painful because I wrote this23:28
rm_workme from two years ago is dumb and I hate him23:28
rm_work(unless I figure out why he did this and it makes sense, but then he's still dumb for not documenting it)23:29
rm_workbut I'm leaning towards that guy just being a jerk23:29
johnsomI think most of it is going API->provider->controller worker schemas23:30
rm_workyeah <_<23:30
johnsomIdeally, at some point in the future, v2 will consume provider format directly23:30
johnsomYou know, when we have that tech debt/refactor time to burn23:31
rm_workheh yeah so I have a patch ready but need to test this somehow23:35
rm_workunit tests are ... hmm23:35
rm_workhmmm did we change our functional tests at some point to actually require config? :/23:36
rm_workgetting this error on like 900+ tests: octavia.common.exceptions.ProviderNotFound: Provider 'amphora' was not found23:38
rm_worklooking into it i guess, since I want to see if I can write a functional test for this23:39
haleybrm_work: i guess you'll have to run the ovn provider :p23:40
johnsomhaleyb Certainly simplifies the case. assert (True, NotImplemented)23:41
* johnsom Thinks you walked into that one23:41
haleybless tests == more time for [snoozing, drinking, ...]23:42
johnsomSo I just did a fresh check out:23:43
johnsom  functional: commands succeeded23:43
johnsom  congratulations :)23:43
rm_workless tests == less time for [snoozing, drinking, ...] because I am debugging weird shit that someone broke with their patch that I didn't notice because there were no tests23:43
johnsomI wonder if you have a config file and some test(s) is picking it up instead of using a fixture?23:43
rm_workyeah maybe23:43
rm_workjohnsom: the only thing i'll say is, this removes the provider's choice as to whether to make gratuitous calls on "no perceived change"23:45
rm_workwe decide not to in Amphora, but maybe some provider actually would want to send all those through (though the major reason that comes to mind is their system being buggy in the backend and needing to allow that to force through again)23:46
johnsomHmm, that is a good point23:46
rm_workbut -- I also DON'T REALLY CARE THAT MUCH sooooo23:49
rm_workI would rather close this hole and let providers request we allow forcing it through or something -- which would be a change we wouldn't need to backport? :P23:49
johnsomThose buggy drivers can add a "repush all" feature. lol23:49
johnsomWhich, I guess we have, called failover23:50
rm_workhmm looks like there was a failover bug in ampv223:51
rm_worki hit the merge button a bit ago on that23:51
rm_worki am about to swap to v2 this week... hoping there aren't more of those lurking :/23:52
rm_workdebating putting that off23:52
rm_workthough I just committed to it in our sprint planning :D23:52
johnsomAh, was going to look at that right after I finished with the priority list (which is about now)23:52
rm_worki double-checked to make sure it did really match what we had in v1, and that it looked like it could matter23:53
johnsomI am kind of wondering why the v2 tempest job didn't catch that23:53
rm_workwould failover an amp, one amp would be reachable?23:54
rm_workor does it do an entire failover WITH a traffic check now?23:54
rm_worklast I checked our failover didn't have traffic checks, but maybe that was added (I remember I had an outstanding CR for a long time on that)23:55
johnsomWell, the base scenario job is standalone.23:55
johnsomMaybe that is the case, and probably should be fixed...23:55
johnsomHow do you test failover if you don't test traffic?????23:55
rm_work /shrug23:57
rm_worknew amp comes up? :P23:58
rm_workalso need to figure out why this fails:
johnsomHmm, maybe the only failover test we have with standalone is in the API suite23:58
rm_workbecause that is what i'd like to merge downstream ahead of time23:58
johnsomYeah, I think that is the case. Only in API which runs no-op. Or only in the active/standby suite.23:59
johnsomNo test joy23:59
johnsomI wonder if I have an old dusty patch with that in it????23:59

Generated by 2.17.2 by Marius Gedminas - find it at!