| *** premsankar has quit IRC | 00:13 | |
| *** EricAdamsZNC has quit IRC | 01:29 | |
| *** EricAdamsZNC has joined #openstack-kuryr | 01:31 | |
| *** EricAdamsZNC has quit IRC | 02:13 | |
| *** EricAdamsZNC has joined #openstack-kuryr | 02:18 | |
| *** gkadam has joined #openstack-kuryr | 04:06 | |
| *** gcheresh_ has joined #openstack-kuryr | 05:22 | |
| *** gcheresh_ has quit IRC | 05:30 | |
| *** janonymous has joined #openstack-kuryr | 05:48 | |
| *** janki has joined #openstack-kuryr | 05:51 | |
| *** gcheresh_ has joined #openstack-kuryr | 06:26 | |
| *** pcaruana has joined #openstack-kuryr | 07:02 | |
| openstackgerrit | Luis Tomas Bolivar proposed openstack/kuryr-kubernetes master: Pools support with Network Policies https://review.openstack.org/634674 | 07:27 |
|---|---|---|
| *** janki is now known as janki- | 07:27 | |
| *** janki- is now known as janki | 07:27 | |
| *** janki is now known as janki|lunch | 07:44 | |
| *** yboaron has quit IRC | 07:56 | |
| openstackgerrit | Ashish Billore proposed openstack/kuryr-kubernetes master: Update HA doc with corections and minor fixes https://review.openstack.org/635446 | 07:58 |
| *** ccamposr has joined #openstack-kuryr | 08:22 | |
| *** celebdor has joined #openstack-kuryr | 08:47 | |
| *** yboaron has joined #openstack-kuryr | 09:00 | |
| *** oanson has joined #openstack-kuryr | 09:51 | |
| openstackgerrit | Maysa de Macedo Souza proposed openstack/kuryr-kubernetes master: Fix SG rules on targetPort update https://review.openstack.org/635039 | 10:54 |
| *** ccamposr has quit IRC | 11:49 | |
| *** maysams has joined #openstack-kuryr | 12:03 | |
| maysams | hey dulek | 12:03 |
| dulek | maysams, ltomasbo: Okay, so https://review.openstack.org/#/c/635039 is about fetching service or endpoint again in case LBaaSState was updated by another handler? | 12:05 |
| ltomasbo | dulek, checking... | 12:05 |
| dulek | ltomasbo: _get_lbaas_state and _set_lbaas_state are only used in on_present() IIUC. | 12:05 |
| dulek | So this is fundamentally broken IMO and still prone to race conditions. | 12:06 |
| ltomasbo | dulek, why? | 12:06 |
| ltomasbo | on_present() is called on creation/modification, right? | 12:07 |
| dulek | But maybe I've led my conclusion too far. :P maysams, so it's trying to protect from such case? When second handler updated state? | 12:07 |
| dulek | ltomasbo: Yup! | 12:07 |
| ltomasbo | dulek, problem (I suppose) is that svc code depends on the annotations both on services and endpoints | 12:07 |
| maysams | dulek: this patch is about updating the endpoint when the Spec has changes | 12:07 |
| dulek | maysams: Can you describe the problematic situation on the handlers level? | 12:09 |
| dulek | Maybe I'm misunderstanding something here. | 12:09 |
| maysams | dulek: okay | 12:10 |
| ltomasbo | dulek, I I get the svc code right, LBaaSSpecHandler is watching on the services status and it is annotating both svc spec and endpoint specs | 12:10 |
| ltomasbo | dulek, then, the LoadBalancerHandler checks if the endpoint annotated spec matches with the endpoint annotated status | 12:10 |
| ltomasbo | and does the required action to move the status to what is reflected on the spec | 12:10 |
| ltomasbo | maysams, am I right? | 12:11 |
| ltomasbo | so, it seems that targetPort information was not included, therefore, we could not react to it | 12:11 |
| maysams | ltomasbo: exactly! You're right | 12:12 |
| dulek | ltomasbo, maysams: I'm worried about this: https://review.openstack.org/#/c/635039/5/kuryr_kubernetes/controller/handlers/lbaas.py@652 | 12:13 |
| dulek | We retrieve that, but we still don't have a guarantee that LBaaSSpecHandler had finished. | 12:14 |
| ltomasbo | dulek, ohh, yes... I just actually commented on that | 12:14 |
| ltomasbo | if there is an update of the endpoint object, the on_present method should be re-executed, right? | 12:15 |
| ltomasbo | so, we should not need that there, right? | 12:15 |
| dulek | ltomasbo: Yup, exactly. | 12:15 |
| ltomasbo | same for line 670-674 | 12:15 |
| maysams | but there is a problem | 12:15 |
| dulek | maysams mentioned that there's no way to tell if that's the on_present we should process or not. | 12:16 |
| dulek | And that's the issue I wanted to solve now. :) | 12:16 |
| ltomasbo | dulek, maysams: that is why it should not be updated, right? | 12:18 |
| dulek | maysams: You're aware of the compare&swap mechanism in K8s API? | 12:18 |
| dulek | maysams: And resourceVersion. | 12:19 |
| ltomasbo | the endpoints annotation spec is only written by the other class | 12:19 |
| dulek | Maybe that helps? | 12:19 |
| ltomasbo | and the LoadBalancer one is only updating the lbaas state annotation | 12:19 |
| ltomasbo | my assumption is, if the endpoint annotation was modified, then the on_present will be triggered, right? | 12:20 |
| ltomasbo | and that will do the corrective actions, and annotate the state | 12:20 |
| ltomasbo | if, by any chance, it was not using the last spec, another on_present() will be triggered | 12:21 |
| ltomasbo | and that one will again see the mismatch between spec and status annotation | 12:21 |
| ltomasbo | or am I missing something? | 12:21 |
| maysams | another on present will be triggered, but no members will be created anymore | 12:21 |
| maysams | because they were already created with the wrong SG | 12:21 |
| maysams | stated in the endpoints annotation | 12:22 |
| ltomasbo | maysams, ok, I think I'm starting to get it | 12:22 |
| ltomasbo | there is an initial on_present() when the new endpoints are created/modified | 12:23 |
| ltomasbo | and that is running with the older spec | 12:23 |
| maysams | yes | 12:24 |
| ltomasbo | as the other svc handler has not already update the spec | 12:24 |
| maysams | yes | 12:24 |
| ltomasbo | ok, anyway, the current code will not solve the issue, right? it is just expecting that by the time the add_new_members method is executed, the other handler has completed the annotations | 12:24 |
| maysams | ltomasbo, right | 12:25 |
| maysams | ltomasbo, dulek, and I don't see any condition that could be checked in other to raise a resourcenoready exception | 12:26 |
| maysams | s/other/order | 12:26 |
| dulek | maysams: There is one way to make it a bit better. | 12:27 |
| dulek | maysams: Instead of fetching new version you can raise ResourceNotReady if update with resourceVersion will not succeed. | 12:27 |
| *** janonymous has quit IRC | 12:28 | |
| dulek | That's way better than fetching it immediately. But doesn't solve everything. | 12:28 |
| dulek | maysams: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency - this seems to explain the concept. | 12:30 |
| dulek | Basically you'll be able to state that you want to update the annotation only if it haven't changed from the object you currently process in on_present. | 12:30 |
| dulek | But in general I think the culprit here is that we update a resource from a handler that isn't watching it, right? | 12:30 |
| maysams | dulek, yes | 12:31 |
| maysams | dulek, so by using resourceversion I will be implementing a nicer version of my patch, but it still do not fix the problem | 12:33 |
| maysams | right? | 12:33 |
| dulek | maysams: Aww, I don't see a solution here immediately. | 12:33 |
| dulek | maysams: I think so, yes. | 12:34 |
| maysams | dulek, thank you for your suggestion | 12:34 |
| maysams | I will try to think on ways to fix this | 12:35 |
| ltomasbo | maysams, now when the svc handler updates the spec, with your patch it will have a different targetPort, right? | 12:41 |
| ltomasbo | perhaps we can use that to re-check what members needs to be added/removed | 12:41 |
| ltomasbo | maysams, maybe here: https://github.com/openstack/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/handlers/lbaas.py#L361 | 12:43 |
| ltomasbo | and then check at line 390 if we need to continue or add it if the target port has changed | 12:44 |
| maysams | ltomasbo: but how do we know on the LoadBalancerHandler side that the spec was updated? | 12:48 |
| ltomasbo | maysams, you mean when the LBaaSSpecHandler annotates the endpoints, a new on_present() is not triggered on the LoadBalancerHander? | 12:49 |
| maysams | ltomasbo: yes, yes.. you're right | 12:51 |
| maysams | sry | 12:51 |
| maysams | yup, I think that might work | 12:51 |
| ltomasbo | fingers crossed! :) | 12:51 |
| maysams | ;-) | 12:52 |
| maysams | thank you! | 12:52 |
| ltomasbo | your welcome! thank you for taking care of it! | 12:52 |
| *** gkadam has quit IRC | 13:02 | |
| *** rh-jelabarre has joined #openstack-kuryr | 13:03 | |
| *** yboaron_ has joined #openstack-kuryr | 13:32 | |
| *** yboaron has quit IRC | 13:35 | |
| *** janki|lunch has quit IRC | 13:57 | |
| *** yboaron_ has quit IRC | 14:08 | |
| *** yboaron_ has joined #openstack-kuryr | 14:13 | |
| maysams | :q | 14:21 |
| *** irclogbot_1 has joined #openstack-kuryr | 14:30 | |
| *** gcheresh_ has quit IRC | 15:23 | |
| *** maysams has quit IRC | 15:33 | |
| *** yboaron_ has quit IRC | 15:35 | |
| *** yboaron_ has joined #openstack-kuryr | 15:36 | |
| *** yboaron_ has quit IRC | 16:04 | |
| *** yboaron_ has joined #openstack-kuryr | 16:05 | |
| *** yboaron_ has quit IRC | 16:05 | |
| *** pcaruana has quit IRC | 16:21 | |
| *** maysams has joined #openstack-kuryr | 16:52 | |
| *** premsankar has joined #openstack-kuryr | 17:03 | |
| dulek | ltomasbo: This patch from yboaron seems promising: https://review.openstack.org/#/c/632684 | 17:17 |
| dulek | ltomasbo: I think we can merge it. | 17:18 |
| ltomasbo | dulek, checking... | 17:18 |
| dulek | ltomasbo: Thanks, hopefully this will solve issues with test_service_pod. | 17:20 |
| ltomasbo | fingers crossed | 17:20 |
| dulek | Now that I think of it… Had anyone even checked if Python's kubernetes client is thread safe on exec? xD | 17:20 |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Remove dragonflow job from experimental jobs https://review.openstack.org/635581 | 17:30 |
| *** aojea has joined #openstack-kuryr | 18:03 | |
| openstackgerrit | Merged openstack/kuryr-tempest-plugin master: Rerun test in a single thread if multi-thread failed https://review.openstack.org/632684 | 18:55 |
| *** aojea has quit IRC | 20:26 | |
| *** celebdor has quit IRC | 20:37 | |
| openstackgerrit | Maysa de Macedo Souza proposed openstack/kuryr-kubernetes master: Fix SG rules on targetPort update https://review.openstack.org/635039 | 21:17 |
| openstackgerrit | Maysa de Macedo Souza proposed openstack/kuryr-kubernetes master: Fix SG rules on targetPort update https://review.openstack.org/635039 | 21:21 |
| *** maysams has quit IRC | 21:39 | |
| *** EricAdamsZNC has quit IRC | 22:03 | |
| *** EricAdamsZNC has joined #openstack-kuryr | 22:04 | |
| *** takamatsu has quit IRC | 23:19 | |
| *** celebdor has joined #openstack-kuryr | 23:27 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!