| *** yushiro has joined #openstack-fwaas | 00:44 | |
| xgerman_ | reedip, yushiro, sridark I think this ^^ might fox our gate failures - please review + comment | 00:44 |
|---|---|---|
| reedip | I hope it doesnt fox our gates :) | 00:44 |
| reedip | checking | 00:46 |
| reedip | xgerman_ : doesnt the check validate if we are Unable to delete a FWG | 00:47 |
| yushiro | xgerman_, Good morning. I checked your patch (https://review.openstack.org/#/c/494034/1) and reviewed. | 00:47 |
| reedip | yushiro : we are missing a test case for it, right ? | 00:48 |
| xgerman_ | reedip we don’t have that count check on the other deletes + I think it would still throw an exception if it can’t delete — all I removed is the Not Found… | 00:48 |
| reedip | xgerman_ : I would like to understand why we are getting a 404 in the first place. If the FWG is already deleted, then its entry shouldnt exist in the DB, right? | 00:49 |
| yushiro | reedip, ah yes. | 00:49 |
| reedip | xgerman_ if we are calling a delete on a FWG which is already deleted, we should get 404, isnt it ? | 00:51 |
| yushiro | xgerman_, reedip If fwg has already been deleted, context.session.query( | 00:52 |
| yushiro | FirewallGroup).filter_by(id=id) returns None or [], right? | 00:52 |
| xgerman_ | I usually return not 404 in that case but it’s debatavle | 00:52 |
| xgerman_ | we put delete on it | 00:52 |
| reedip | yushiro : would return None, IIRC | 00:52 |
| reedip | xgerman_ : well if we have a check for the fwg at an earlier stage , i.e. in the extension , then we can remove the check in the DB | 00:53 |
| reedip | https://github.com/openstack/neutron-fwaas/search?utf8=%E2%9C%93&q=%22def+delete_firewall_group%22&type= | 00:53 |
| xgerman_ | I know that if we return 404 we make life difficult for people who have concurremcy issues callign us | 00:54 |
| xgerman_ | so I am in favor of eating that error — | 00:54 |
| reedip | xgerman_ agreed that race conditions would be affected on parallel deletes. I am not against it :) | 00:55 |
| reedip | * against eating up the error | 00:56 |
| reedip | xgerman_, yushiro : Ideally if the ID doesnt exist, we might end up with an error in https://github.com/openstack/neutron-fwaas/blob/9d9799f20ca202ceeb8354a6817c856c26434869/neutron_fwaas/services/firewall/fwaas_plugin_v2.py#L355 or https://github.com/openstack/neutron-fwaas/blob/9d9799f20ca202ceeb8354a6817c856c26434869/neutron_fwaas/services/firewall/drivers/linux/iptables_fwaas_v2.py#L110 | 00:59 |
| yushiro | _make_firewall_group_dict_with_rules() | 01:01 |
| yushiro | it calls get_firewall_group() and returns 404 | 01:01 |
| reedip | would the make_firewall_group_dict_with_rules be called for Delete? I thought it was called for Update/Create etc. | 01:02 |
| reedip | It basically returns the data back to the user, IIRC | 01:02 |
| yushiro | https://github.com/openstack/neutron-fwaas/blob/9d9799f20ca202ceeb8354a6817c856c26434869/neutron_fwaas/services/firewall/fwaas_plugin_v2.py#L358 | 01:02 |
| reedip | aah | 01:03 |
| reedip | :) | 01:03 |
| reedip | done then | 01:03 |
| yushiro | Before entering DB layer, _make_firewall_group_dict_with_rules is definitely called. However, race condition will be occurred. | 01:05 |
| reedip | yushiro : with respect to the race condition,there are 2 conditions | 01:06 |
| reedip | yushiro: if the first request deletes the FWG and the second request just arrives at checking the ID, then it would get 404, which is expected | 01:07 |
| reedip | yushiro: if both calls pass the get_firewall_group() as the FWG was not yet deleted, then xgerman_ 's fix fixes the issue :) | 01:07 |
| xgerman_ | lucky me ;-) | 01:08 |
| xgerman_ | yeah, I really like our gates to be fixed so we can have the release stuff merged | 01:09 |
| yushiro | Maybe yes. Simply, delete_firewall_group in DB layer is called at same time for same ID. | 01:09 |
| yushiro | I just wrote UTs for it. http://paste.openstack.org/show/618458/ | 01:19 |
| yushiro | Please let me ask more. | 01:20 |
| openstackgerrit | Reedip proposed openstack/neutron-fwaas master: consume load_class_by_alias_or_classname from neutron-lib https://review.openstack.org/487139 | 01:21 |
| reedip | xgerman_ , yushiro : Meanwhile in neutron-lib migration ^^ | 01:22 |
| yushiro | reedip, thanks. will check. | 01:29 |
| yushiro | context.session.query(FirewallGroup).filter_by(id=id) returns oslo_db.sqlalchemy.orm.Query object even if non-exist ID. | 01:30 |
| reedip | but we are calling delete on it | 01:31 |
| yushiro | Then, if executed ".delete()", it returns 0 | 01:31 |
| yushiro | hmm, sorry. I still don't understand correctly. If race condition occurred, both API will return 204 ? | 01:33 |
| reedip | Nope | 01:34 |
| reedip | with the subtransaction=True, we would have only one call for WRITE on the DB | 01:34 |
| reedip | IIRC | 01:34 |
| reedip | so by the time the second one comes to delete,the first one would have deleted it and ... 404 | 01:35 |
| yushiro | 1st: delete DB successfully and return 204. 2nd: already removed and returns 404 This is current behavior. | 01:36 |
| yushiro | In xgerman_'s patch, if non-exist fwg_id is specified, it doesn't return FirewallGroupNotFound. | 01:37 |
| yushiro | ^^ in DB layer's behavior | 01:37 |
| reedip | Yes, then we would have 204 | 01:37 |
| reedip | so if the a Delete Request checks that a FWG exists, it goes to delete the FWG, but before it deletes it, someone else has already deleted it, then return 204 | 01:38 |
| yushiro | If so, 1st: 204, 2nd: 204 | 01:39 |
| reedip | If the delete request cannot find the FWG in the first check, 404 | 01:39 |
| yushiro | Hmm, it seems different behavior in each layer... | 01:42 |
| reedip | :) Depends on which layer is interacting with the user .. its User experience after all | 01:44 |
| yushiro | hmm, each layer(plugin, DB) raises exceptions (NotFound, Conflict, ...) for users. | 02:11 |
| yushiro | So, I think it's better to fix tempest plugin... | 02:13 |
| yushiro | but it is OK in DB consistency perspective. | 02:17 |
| openstackgerrit | Vu Cong Tuan proposed openstack/neutron-fwaas master: FW rule applied incorrectly if port specified is a range https://review.openstack.org/443385 | 02:52 |
| openstackgerrit | Yushiro FURUKAWA proposed openstack/neutron-fwaas master: Don't return 404 when deleting a non-existant FWG https://review.openstack.org/494034 | 03:04 |
| yushiro | Just added UT | 03:04 |
| *** yushiro is now known as yushiro_lunch | 03:05 | |
| *** vishwana_ has joined #openstack-fwaas | 03:18 | |
| *** vishwanathj has quit IRC | 03:21 | |
| openstackgerrit | Vu Cong Tuan proposed openstack/neutron-fwaas master: FW rule applied incorrectly if port specified is a range https://review.openstack.org/443385 | 04:27 |
| *** reedip has quit IRC | 05:23 | |
| *** reedip has joined #openstack-fwaas | 05:37 | |
| *** SridarK has joined #openstack-fwaas | 05:37 | |
| *** yushiro_lunch is now known as yushiro | 05:41 | |
| yushiro | xgerman_, SridarK Let me sync my PTG status | 05:41 |
| yushiro | xgerman_, SridarK I can go to PTG!! I passed travel support program :) | 05:42 |
| SridarK | yushiro: oh great | 05:45 |
| SridarK | yushiro: when are u back from PTO ? | 05:46 |
| yushiro | I came back since yesterday's evening. | 05:47 |
| yushiro | My PTO was 11th - 16th Aug. | 05:47 |
| yushiro | oops, 15th | 05:47 |
| SridarK | yushiro: ah ok | 05:48 |
| SridarK | we discussed some PTG planning in our IRC | 05:48 |
| SridarK | https://etherpad.openstack.org/p/neutron-queens-ptg | 05:48 |
| SridarK | few updates there in FWaaS section | 05:48 |
| SridarK | yushiro: i got back last night too so catching up today | 05:49 |
| SridarK | :-) | 05:49 |
| yushiro | SridarK, Aha, you too? :) Yes, I saw meeting log. | 05:50 |
| yushiro | and thanks for the link of etherpad. | 05:51 |
| SridarK | yushiro: no worries - we will focus on the Horizon patch. I sent a reminder to Sarath to look into Akihiro's comments as well and we can do some testing as well | 05:58 |
| yushiro | SridarK, OK. BTW, xgerman_ has posted https://review.openstack.org/#/c/494034 . It may be able to avoid tempest test failure. Could you review it ? | 06:03 |
| SridarK | yushiro: hmm so is this the root cause of the failures | 06:07 |
| SridarK | i have not had a chance to check this | 06:07 |
| SridarK | In a tempest scenario - it is odd that the delete has happened before the delete is called | 06:07 |
| SridarK | seems suspicious ? | 06:08 |
| SridarK | I hope we are not masking an issue | 06:08 |
| SridarK | But yes i think normally it looks ok to me that if we cannot find the fwg to be deleted - so be it | 06:09 |
| yushiro | SridarK, Yes, I'm not sure what is root cause ( Not researched yet ) | 06:13 |
| yushiro | SridarK, TBH, I think plugin/DB layer should be a same behavior. | 06:14 |
| SridarK | yushiro: yes agreed - ok i am fine with it - let me sync with xgerman_ in the AM and put in the review | 06:18 |
| yushiro | SridarK, Thank you. I also check tempest test cases. | 06:18 |
| yushiro | Of course v2 horizon patch :) | 06:18 |
| *** SridarK has quit IRC | 07:51 | |
| *** yamamoto has joined #openstack-fwaas | 08:33 | |
| *** yamamoto has quit IRC | 08:49 | |
| reedip | yushiro: congrats | 08:55 |
| reedip | :) | 08:55 |
| yushiro | reedip, thanks ! | 10:34 |
| openstackgerrit | Elena Ezhova proposed openstack/neutron-fwaas master: [DevStack] Configure iptables_v2 firewall driver for FWaaS V2. https://review.openstack.org/494167 | 10:54 |
| openstackgerrit | Elena Ezhova proposed openstack/neutron-fwaas master: [DevStack] Configure iptables_v2 firewall driver for FWaaS V2. https://review.openstack.org/494167 | 12:08 |
| openstackgerrit | Inessa Vasilevskaya proposed openstack/neutron-fwaas master: FirewallGroupPortInvalidProject can be raised now https://review.openstack.org/494198 | 13:03 |
| xgerman_ | o/ | 13:57 |
| amotoki | can some fwaas-core approve https://review.openstack.org/493666 (relnote build fix) ? | 14:00 |
| openstackgerrit | Elena Ezhova proposed openstack/neutron-fwaas master: [DevStack] Configure iptables_v2 firewall driver for FWaaS V2. https://review.openstack.org/494167 | 14:30 |
| *** reedip_ has joined #openstack-fwaas | 15:21 | |
| *** reedip_ has quit IRC | 15:42 | |
| openstackgerrit | Merged openstack/neutron-fwaas master: fix releasenotes build https://review.openstack.org/493666 | 16:22 |
| openstackgerrit | Andrea Frittoli proposed openstack/neutron-fwaas master: Get default client config from config module https://review.openstack.org/494275 | 17:05 |
| openstackgerrit | Elena Ezhova proposed openstack/neutron-fwaas master: [DevStack] Configure iptables_v2 firewall driver for FWaaS V2. https://review.openstack.org/494167 | 17:16 |
| xgerman_ | things are merging again so we might not need my patch after all… | 20:20 |
| openstackgerrit | Merged openstack/neutron-fwaas master: Update reno for stable/pike https://review.openstack.org/492908 | 20:27 |
| *** vishwana_ has quit IRC | 21:03 | |
| *** vishwanathj has joined #openstack-fwaas | 21:04 | |
| *** yamamoto has joined #openstack-fwaas | 21:44 | |
| *** yamamoto has quit IRC | 21:45 | |
| *** yamamoto has joined #openstack-fwaas | 21:51 | |
| *** vishwana_ has joined #openstack-fwaas | 21:54 | |
| *** yamamoto has quit IRC | 21:58 | |
| *** vishwanathj has quit IRC | 21:58 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!