| *** mrostecki has quit IRC | 00:41 | |
| *** mrostecki has joined #openstack-kuryr | 00:42 | |
| *** celebdor has quit IRC | 01:09 | |
| *** janki has joined #openstack-kuryr | 05:05 | |
| *** yboaron_ has joined #openstack-kuryr | 06:04 | |
| *** janki has quit IRC | 06:08 | |
| *** gcheresh_ has joined #openstack-kuryr | 06:08 | |
| *** janki has joined #openstack-kuryr | 06:08 | |
| *** lxkong has quit IRC | 06:52 | |
| *** janki has quit IRC | 06:54 | |
| *** janki has joined #openstack-kuryr | 06:54 | |
| *** ccamposr has joined #openstack-kuryr | 07:14 | |
| *** gkadam__ has joined #openstack-kuryr | 07:21 | |
| *** lxkong has joined #openstack-kuryr | 07:21 | |
| *** pcaruana has joined #openstack-kuryr | 07:25 | |
| *** maysams has joined #openstack-kuryr | 08:02 | |
| *** celebdor has joined #openstack-kuryr | 08:12 | |
| dulek | maysams: You were offline yesterday evening so sending again: http://paste.openstack.org/show/747373/ | 08:38 |
|---|---|---|
| dulek | maysams: 3 out of 8 tests failed, and one might just have been my DNS hiccup. | 08:39 |
| dulek | maysams: But 5 are totally fine, which is great, because it seems we got most of the stuff right. :) | 08:39 |
| maysams | dulek: That's great news | 08:40 |
| openstackgerrit | Luis Tomas Bolivar proposed openstack/kuryr-kubernetes master: Add support for svc with text targetPorts https://review.openstack.org/641598 | 08:41 |
| maysams | dulek, thanks for sharing it with me | 08:41 |
| maysams | dulek, I will later take a look on those failing tests.. maybe we're missing something on the NP support | 08:42 |
| maysams | ddulek: which one do you think it was the dns hiccup? | 08:42 |
| dulek | maysams: "should support allow-all policy" | 08:42 |
| dulek | maysams: The last two had something like this in NP: | 08:43 |
| dulek | Port: "serve-80" | 08:43 |
| dulek | I guess we again missed the fact that ports can also be referenced as names, not only as numbers. | 08:43 |
| dulek | But I'm not sure. ;) | 08:43 |
| maysams | dulek, you're probably right | 08:50 |
| dulek | maysams: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/network/network_policy.go#L281-L356 | 08:51 |
| dulek | maysams: Those are the two tests. | 08:51 |
| maysams | dulek: yup, I was checking them | 08:52 |
| maysams | dulek: and it does not provide any detailed msg of why it's failling, right? | 08:53 |
| dulek | "should allow egress access on one named port": "Pod client-a should not be able to connect to service svc-server, but was able to connect." | 08:53 |
| dulek | "should allow ingress access on one named port": "Pod client-b should not be able to connect to service svc-server, but was able to connect." | 08:54 |
| dulek | maysams: I see one 500 from Neutron… | 08:56 |
| maysams | dulek, thanks.. have the k8s resources gotten deleted? | 08:57 |
| dulek | maysams: K8s? Yes, tests delete whole namespace, so all K8s stuff is gone. | 08:58 |
| maysams | hmm | 08:58 |
| maysams | it would be nice to double check what is present in the ports field of the NP | 08:59 |
| dulek | maysams: "(pymysql.err.IntegrityError) (1451, u'Cannot delete or update a parent row: a foreign key constraint fails (`neutron`.`securitygroupportbindings`, CONSTRAINT `securitygroupportbindings_ibfk_2…" | 08:59 |
| dulek | That's the Neutron issue. | 08:59 |
| dulek | I guess we've found a Neutron bug as well. xD | 09:00 |
| dulek | maysams: If I figure out how to run just one test, we can do it. | 09:00 |
| dulek | maysams: I have a quick call now, will be back in 15-30 minutes. | 09:00 |
| maysams | dulek, ok ok | 09:00 |
| maysams | dulek: I'm almost certain that the problem it's because we're not handling the possibility(https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.13/#networkpolicyport-v1beta1-extensions) of named ports | 09:09 |
| *** alisanhaji has joined #openstack-kuryr | 09:10 | |
| maysams | we're retrieving the port from the NP and requesting a sg rule creation with it | 09:11 |
| maysams | https://github.com/openstack/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/drivers/network_policy.py#L325 | 09:11 |
| maysams | https://github.com/openstack/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/drivers/utils.py#L238 | 09:11 |
| dulek | maysams: Shouldn't I see some tracebacks then? | 09:24 |
| maysams | dulek: hmm maybe | 09:29 |
| maysams | dulek: don't we have any info on kuryr log? | 09:29 |
| dulek | maysams: I'll look through it again, but haven't seen any. | 09:31 |
| maysams | okay | 09:31 |
| maysams | and you did you see the neutron error on neutron service logs? | 09:31 |
| dulek | maysams: Got it! | 09:41 |
| dulek | BadRequest: Invalid value for port serve-80 | 09:41 |
| dulek | maysams: And this is the NP in question: http://paste.openstack.org/show/747384/ | 09:42 |
| dulek | maysams: And that Neutron 500 is above - it's some MySQL error about securitygroupportbinding table. | 09:42 |
| maysams | dulek, Is this badrequest raised from neutron? | 09:44 |
| dulek | maysams: Yes, yes, it's from Neutron. | 09:44 |
| dulek | maysams: http://paste.openstack.org/show/747385/ - full traceback. | 09:45 |
| maysams | dulek, good. So, that was definitely the problem | 09:45 |
| dulek | maysams: Yup, for sure! | 09:45 |
| maysams | dulek, Will you open a bug for this? I can fix it, if it's ok for you | 09:49 |
| dulek | maysams: Sure, opening one now. | 09:49 |
| dulek | maysams: I prefer you'll fix it, I don't feel that comfortable with NP code. :P | 09:50 |
| maysams | dulek: okay :P | 09:50 |
| *** yboaron_ has quit IRC | 09:52 | |
| dulek | maysams: https://bugs.launchpad.net/kuryr-kubernetes/+bug/1818983 | 09:54 |
| openstack | Launchpad bug 1818983 in kuryr-kubernetes "NetworkPolicy doesn't support named ports" [High,Confirmed] - Assigned to Maysa de Macedo Souza (maysa) | 09:54 |
| maysams | dulek: Thank you. | 09:55 |
| ltomasbo | ohh, we also need named port for policies! | 10:24 |
| ltomasbo | maysams, dulek: serve-80?? | 10:24 |
| ltomasbo | where that is defined? | 10:24 |
| dulek | ltomasbo: I believe that's the name of the port on the *Pod*. | 10:25 |
| dulek | ltomasbo: That's why I wanted to start discussing your patch, I'm confused if it's right. | 10:25 |
| ltomasbo | dulek, maysams: and will that be as simple as, if it is a string, just do .split("-")[-1] | 10:26 |
| dulek | ltomasbo: No. xD | 10:26 |
| ltomasbo | umm | 10:26 |
| dulek | ltomasbo: I'll show you, wait a second. | 10:26 |
| ltomasbo | you mean that allows traffic to go out on port serve-80 | 10:26 |
| ltomasbo | how do you know in the local pod what ports are opened? | 10:27 |
| ltomasbo | or you mean we should also only open the ports that are opened in the pod definition? | 10:27 |
| ltomasbo | I think we are not caring at all about what the pod definition says about ports... | 10:27 |
| maysams | dulek: I believe ltomasbo patch is okay according to this https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.13/#serviceport-v1-core | 10:28 |
| dulek | ltomasbo: So in those NP tests here's the NP definition: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/network/network_policy.go#L294-L298 | 10:28 |
| dulek | ltomasbo: And this is what it should match: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/network/network_policy.go#L473 | 10:29 |
| ltomasbo | dulek, yep, so I guess we have a bigger problem there, as we don't use any information regarding the ports on the pod spec in kuryr-kubernetes, right? | 10:30 |
| ltomasbo | maysams, umm, so I got it half right? | 10:30 |
| ltomasbo | based on the definition it is equal to the port field when it is not specified | 10:31 |
| ltomasbo | on the pod | 10:31 |
| dulek | ltomasbo: Yeah, but if it's a string, then it should be looked up in that pod's ports, by names. | 10:31 |
| ltomasbo | dulek, yep, and if not there, then assign it the port value on the svc spec | 10:32 |
| ltomasbo | dulek, ok, so that is also assuming all the pods pointed have the very same ports definition | 10:33 |
| ltomasbo | not only the port, but also the names | 10:33 |
| ltomasbo | which makes sense | 10:33 |
| dulek | ltomasbo: Yes, you're right. | 10:33 |
| ltomasbo | and seems that at the end of the day, you are using the same name as in the svc spec | 10:33 |
| ltomasbo | so, what I propose is a half-fix, but it should be enhance with that checking about the pointed pods definition | 10:34 |
| maysams | ltomasbo: yup | 10:34 |
| ltomasbo | and we need to do the same for the network policies... | 10:35 |
| dulek | ltomasbo: We don't have definitions of ports for each pointed pod in endpoints? | 10:35 |
| ltomasbo | dulek, yes we do | 10:35 |
| dulek | ltomasbo: So at least we don't need to fetch pods and so on, all the info is there, we just need to manipulate it correctly, right? | 10:36 |
| ltomasbo | but we have a handler for services and another one for endpoints... | 10:36 |
| openstackgerrit | OpenStack Release Bot proposed openstack/kuryr master: Update master for stable/stein https://review.openstack.org/641627 | 10:36 |
| dulek | Uh, oh. ^ | 10:36 |
| dulek | :P | 10:36 |
| dulek | Had we branched? | 10:36 |
| dulek | ltomasbo: Don't we copy stuff from endpoints into annotation on Services? | 10:37 |
| ltomasbo | dulek, I think it is the other way around actually | 10:39 |
| ltomasbo | maysams, ^^ | 10:39 |
| maysams | ltomasbo: dulek: yup.. is the other way around | 10:40 |
| dulek | Huh? It's LBaaSSpecHandler that waits until lbaas spec is on /services/. | 10:41 |
| dulek | Oh no, you're right. | 10:42 |
| *** yboaron_ has joined #openstack-kuryr | 10:43 | |
| openstackgerrit | Luis Tomas Bolivar proposed openstack/kuryr-kubernetes master: Update documentation about NP handlers needed https://review.openstack.org/641629 | 10:47 |
| ltomasbo | maysams, dulek, https://review.openstack.org/641629 | 10:47 |
| dulek | ltomasbo: Easy stuff first, huh? ;) | 10:48 |
| ltomasbo | :) | 10:49 |
| ltomasbo | dulek, I just fixed it on openshift-ansible... so... | 10:49 |
| *** gkadam__ has quit IRC | 12:32 | |
| *** gkadam__ has joined #openstack-kuryr | 12:32 | |
| *** janki has quit IRC | 12:40 | |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Switch Octavia API calls to openstacksdk https://review.openstack.org/638258 | 12:59 |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Add option to tag Octavia resources created by us https://review.openstack.org/638483 | 12:59 |
| openstackgerrit | Genadi Chereshnya proposed openstack/kuryr-tempest-plugin master: Fixing service connectivity testing https://review.openstack.org/636329 | 13:00 |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Switch Octavia API calls to openstacksdk https://review.openstack.org/638258 | 13:18 |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Add option to tag Octavia resources created by us https://review.openstack.org/638483 | 13:18 |
| *** celebdor has quit IRC | 13:23 | |
| *** rh-jelabarre has joined #openstack-kuryr | 13:32 | |
| *** celebdor has joined #openstack-kuryr | 13:35 | |
| *** janki has joined #openstack-kuryr | 13:36 | |
| *** rh-jelabarre has quit IRC | 13:37 | |
| *** rh-jelabarre has joined #openstack-kuryr | 14:39 | |
| *** gcheresh_ has quit IRC | 15:30 | |
| *** janki has quit IRC | 16:29 | |
| *** rh-jelabarre has quit IRC | 16:38 | |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Switch Octavia API calls to openstacksdk https://review.openstack.org/638258 | 16:38 |
| openstackgerrit | Michał Dulko proposed openstack/kuryr-kubernetes master: Add option to tag Octavia resources created by us https://review.openstack.org/638483 | 16:38 |
| *** yboaron_ has quit IRC | 16:52 | |
| *** premsankar has joined #openstack-kuryr | 17:03 | |
| ltomasbo | dulek, maysams: this seems totally wrong: https://github.com/openstack/kuryr-kubernetes/blob/master/kuryr_kubernetes/controller/handlers/lbaas.py#L264-L282 | 17:05 |
| ltomasbo | dulek, maysams L267 should have a not at the beginning... | 17:05 |
| ltomasbo | dulek, maysams but my question is, how has this worked all this time? | 17:06 |
| maysams | ltomasbo: I'm creating a svc right know to see what is happening | 17:06 |
| dulek | ltomasbo: "_is_lbaas_spec_in_sync". If spec is in sync, then this returns true. | 17:06 |
| ltomasbo | dulek, yes, and if that returns true, should_ignore should return true, as if it is already in sync, there is nothing to do | 17:07 |
| dulek | ltomasbo: Oh yes… | 17:07 |
| dulek | ltomasbo: Heisenbug. Now that you've found it we'll start to see its implications everywhere. | 17:07 |
| dulek | Thanks you ltomasbo… | 17:08 |
| ltomasbo | dulek, but that has been working... I just changed the targetPort from int to string to see if that was enough and saw that no lbaas were created | 17:08 |
| *** celebdor has quit IRC | 17:08 | |
| ltomasbo | lbs | 17:08 |
| ltomasbo | dulek, just adding a not in front of that line, creates them | 17:08 |
| dulek | ltomasbo: Told ya, heisenbug. | 17:09 |
| ltomasbo | but still, how this is working!!! | 17:09 |
| dulek | :D | 17:09 |
| ltomasbo | I check juriarte's env and it is working... | 17:09 |
| ltomasbo | it is before adding the targetPort to the spec port, but still | 17:09 |
| ltomasbo | unless it was simply timing... due to that rule failing for not having endpoints or spec | 17:10 |
| ltomasbo | that has been there for 2 years!! it cannot be... I must be missing something... | 17:11 |
| ltomasbo | maysa, wait a second, why did you check this at line 272: if ports[0].obj_attr_is_set('targetPort') | 17:14 |
| ltomasbo | maysams, should it be if port.obj_attr_is_set('targetPort') instead? | 17:14 |
| ltomasbo | it should not matter for the logic though | 17:15 |
| maysams | ltomasbo, nop | 17:15 |
| maysams | ports refer to lbaas_spec.ports | 17:15 |
| dulek | ltomasbo: It's like that to detect if that's old annotation or new. | 17:16 |
| ltomasbo | umm, ok, I suppose it should be moved out of the if for efficiency though, | 17:17 |
| maysams | Guys, I will need to leave. But will be back in a few min | 17:17 |
| ltomasbo | I'll leave soon too | 17:17 |
| dulek | maysams, ltomasbo: BTW - I haven't found better way to represent int or string than plain string, so let's go that route. | 17:18 |
| ltomasbo | anyway, good news is that, simply moving targetPort to string seems to fix the problem | 17:18 |
| ltomasbo | dulek, my question is, should I update that 'not' in the same patch? | 17:18 |
| ltomasbo | it only happened after I changed that to string | 17:18 |
| dulek | ltomasbo: I don't really know… | 17:19 |
| ltomasbo | before it was working... not sure about the relation yet | 17:19 |
| ltomasbo | and not sure how it is possible that it is working currently | 17:19 |
| ltomasbo | umm I think I got it now... | 17:21 |
| ltomasbo | maybe that function name is a bit misleading | 17:21 |
| ltomasbo | dulek, ^^ seems that what is checking is if whatever the svc handler wrote in the endpoint spec is already in sycn with whatever endpoints already have | 17:21 |
| ltomasbo | so, until that happens, no further actions should happen on the neutron/octavia side | 17:22 |
| dulek | ltomasbo: Oh. | 17:22 |
| ltomasbo | so, probably by changing it to string, it never matches and that is why it was blocked there... | 17:22 |
| ltomasbo | now it makes sense... I knew I must have been missing something | 17:23 |
| *** maysams has quit IRC | 17:23 | |
| ltomasbo | it could not be such a big bug there for 2 years... | 17:23 |
| dulek | ltomasbo: Oh yes, if we start saving string it will never find it. | 17:26 |
| dulek | ltomasbo: Because endpoints have it converted. | 17:26 |
| ltomasbo | dulek, yep! | 17:26 |
| dulek | ltomasbo: Glad you found it, I would spent hours on that. | 17:26 |
| ltomasbo | :) | 17:27 |
| dulek | ltomasbo: Now how to do it properly? Obvious answer would be to always save int, but is it easily reachable? | 17:27 |
| ltomasbo | it was simply not able to believe the 'not' should be missing... | 17:27 |
| ltomasbo | checking... | 17:27 |
| ltomasbo | probably enhancing the _is_lbaas_spec_in_sycn | 17:28 |
| ltomasbo | and if we put 'web' check if that one is defined on the port name | 17:28 |
| ltomasbo | that way we can keep lbaas_spec with string, and just ensure this is ignored until endpoints and endpoints annotations are in sync | 17:29 |
| ltomasbo | actually, at _is_lbaas_spec_in_sync, I'm not sure we actually need the port number | 17:32 |
| ltomasbo | if the port name is there, it should be enough | 17:32 |
| ltomasbo | yep! that is not needed! | 17:33 |
| dulek | ltomasbo: Yup, makes sense. | 17:39 |
| ltomasbo | then, it may be simple to fix... updating my commit... | 17:43 |
| *** gkadam__ has quit IRC | 17:53 | |
| ltomasbo | dulek, dummy question, as I've updated the LBaaSPortSpec (int to string) | 17:53 |
| ltomasbo | dulek, test about the object/hash fail, how are you updating those? | 17:54 |
| dulek | ltomasbo: Just copy the correct hash from tests output and put it where it should be. | 17:54 |
| ltomasbo | ahh ok | 17:54 |
| dulek | ltomasbo: We don't care about that incompatibility as we don't support upgrading from inter-release. | 17:55 |
| ltomasbo | yep | 17:55 |
| dulek | ltomasbo: So inside a single release we assume we can do whatever we want. :P | 17:55 |
| ltomasbo | taht was my understanding | 17:55 |
| ltomasbo | thanks! | 17:56 |
| openstackgerrit | Luis Tomas Bolivar proposed openstack/kuryr-kubernetes master: Add support for svc with text targetPorts https://review.openstack.org/641598 | 17:56 |
| *** irclogbot_1 has joined #openstack-kuryr | 18:03 | |
| *** rh-jelabarre has joined #openstack-kuryr | 18:12 | |
| *** maysams has joined #openstack-kuryr | 18:28 | |
| *** ccamposr has quit IRC | 18:43 | |
| *** pcaruana has quit IRC | 18:46 | |
| *** pcaruana has joined #openstack-kuryr | 19:05 | |
| *** pcaruana has quit IRC | 19:33 | |
| *** alisanhaji has quit IRC | 21:15 | |
| *** premsankar has quit IRC | 22:12 | |
| *** rh-jelabarre has quit IRC | 22:21 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!