| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2020-02-26 | |||
| 14:47:02 | gtema | but anyway wanted to have a session on PTG wrt that | |
| 14:47:35 | mordred | gtema: one quibble - in project_cleanup - you've got code to handle people who are for some reason biased against threads - and that's the default code path ... why not make it use a thread pool by default and make people who want to "avoid" threads do work? (we use threads for chunked uploads to swift whether a user wants them or not, so I'm personally not even convinced we need to support avoiding | |
| 14:47:36 | mordred | them at all) | |
| 14:48:14 | gtema | dtantsur was vomplaining a lot agains that | |
| 14:48:19 | gtema | complaining | |
| 14:48:38 | mordred | one sec - gotta step away for 5 mins... | |
| 14:48:40 | gtema | what you said was a 2nd approach, but he still didn't like it | |
| 14:48:43 | gtema | ok | |
| 14:54:30 | mordred | k. back | |
| 14:54:43 | gtema | good | |
| 14:54:46 | mordred | gtema: so what's the issue - he's opposed to threads for some reason? | |
| 14:55:04 | mordred | oh - I see - kemme read the comments on the patch | |
| 14:55:14 | gtema | yeah, he was agains having it used in the default path | |
| 14:56:49 | mordred | gtema: his original comment was just about allowing the possibilities to pass in a manager: https://review.opendev.org/#/c/700219/7/openstack/cloud/openstackcloud.py@796 | |
| 14:57:07 | gtema | I think he was explaining me more concerns in the private chat, but I lost it and forgot | |
| 14:57:13 | mordred | nod | |
| 14:57:26 | gtema | and then he said - please avoid it completely | |
| 14:57:41 | gtema | we can wait for him then for details | |
| 14:57:56 | mordred | so - I completely agree about the greenlet concerns - because people using greenlet stuff are in a general world of compexity and pain as they have no clue what their code is doing at any time :) | |
| 14:58:05 | mordred | so definitely it's important to give those folks an out | |
| 14:58:11 | gtema | :D | |
| 14:58:35 | mordred | but - I think someone writing some straightforward code should not need to opt-in to what should be normal safe behavior | |
| 14:59:05 | mordred | they shold get the best experience out of the box just by running the cleanup method | |
| 14:59:08 | gtema | I share same opinion, so we are 2 against 1. | |
| 14:59:13 | mordred | and only need to do special things if they are in special circumstances | |
| 14:59:29 | gtema | then post a review to patch and we see what he says | |
| 14:59:40 | mordred | kk | |
| 15:00:00 | mordred | btw - the greenlet concern is good - we should maybe make sure we're providing a similar escape hatch for people for other uses of executors | |
| 15:00:10 | mordred | in fact- I _think_ you can pass an executor to Connection? | |
| 15:00:18 | mordred | one sec- lemme look | |
| 15:01:12 | mordred | nope. it's a TODO | |
| 15:01:29 | mordred | gtema: so - if you look in openstack/cloud/_object_store.py | |
| 15:01:49 | mordred | there's a TODO about making self.__pool_executor configurable - for the folks in the greenlet situation | |
| 15:02:13 | mordred | maybe we should just make that a proper Connection param- then we can use self.__pool_executor in your other code | |
| 15:02:33 | mordred | and it'll also be clear to people using sdk from services how to set up a connection safely | |
| 15:02:52 | gtema | a sec, have a call | |
| 15:02:53 | mordred | (maybe this is why we were considering switching to futurist at one point?) | |
| 15:02:59 | mordred | kk. I'll leave a review | |
| 15:06:06 | gtema | back. | |
| 15:06:09 | gtema | okay, great | |
| 15:06:12 | gtema | thanks | |
| 15:16:43 | mordred | gtema: done. +2 otherwise | |
| 15:16:52 | gtema | great, thanks a lot | |
| 15:16:59 | mordred | gtema: dude - thank you for writing that | |
| 15:17:09 | gtema | welcome | |
| 15:17:10 | mordred | sorry it's taken me so long to review :) | |
| 15:17:20 | gtema | I really need it myself in lots of my projects | |
| 15:17:33 | gtema | no problems, need only to ping people some time ;-) | |
| 15:17:39 | gtema | sometime be nasty | |
| 15:20:54 | gtema | hopefully Vancouver will not be that much affected by Corona | |
| 15:28:32 | mordred | gtema: we should probably review https://review.opendev.org/#/c/679914/ too | |
| 15:29:09 | gtema | oh yeah, I see I was even reviewing it already | |
| 16:00:49 | dulek | mordred: Hi! Can you take a look at https://review.opendev.org/710030 ? I'm not sure how to proceed with this in an openstacksdk'ish way there. | |
| 16:01:37 | dulek | Basically the issue is that in Neutron `If-Match: revision_number=1` is the correct form, so I'd need some header modification to make this thing useful. | |
| 16:01:57 | dulek | Also usage of that header should be restricted to PUT and DELETE calls. | |
| 16:03:13 | mordred | dulek: oh my - what a fun question ... | |
| 16:03:56 | mordred | dulek: I believe we're going to get to invent a new primitive on Resource! | |
| 16:04:54 | dulek | I always engage in fun stuff. | |
| 16:05:24 | dulek | mordred: But as openstacksdk beginner I could use some advice. | |
| 16:14:17 | mordred | dulek: totally. I'm in the "staring off into space looking like I'm doing nothing but actually pondering your issue" state. hopefully soon I'll transition to "looking like someone who has an idea" | |
| 16:16:20 | dulek | mordred: Sure, thanks! | |
| 16:17:24 | openstackgerrit | Merged openstack/python-openstackclient stable/train: Stop silently ignoring invalid 'server create --hint' options https://review.opendev.org/705630 | |
| 16:25:33 | openstackgerrit | Merged openstack/ansible-collections-openstack master: Make an OpenStackModule base class https://review.opendev.org/698044 | |
| 16:34:50 | openstackgerrit | Merged openstack/ansible-collections-openstack master: Cleanup unit test requirements https://review.opendev.org/709113 | |
| 16:54:20 | mordred | dulek: would it be desirable do you think to have some amount of if-match happen automatically? like - if the user has a Network resource locally and goes to commit an update, should sdk automatically add an if_match="revision={current_object.revision}" if the user hasn't added one? | |
| 16:54:41 | mordred | or would that be super unexpected and unwelcome? | |
| 16:55:51 | dulek | mordred: I'd say that would be unwelcome. Neutron is not analyzing anything here, just comparing numbers and sometimes people don't care if somebody changed something in-between, they just want to rename. | |
| 16:55:58 | mordred | nod | |
| 16:56:20 | dulek | mordred: In our case we use it to make sure we won's lose an update when updating allowed_address_pairs. | |
| 16:56:30 | mordred | yah | |
| 17:10:39 | mordred | dulek: ok - so - I'm just gonna talk out loud here for a bit - this may be bong | |
| 17:14:18 | mordred | I think what you probably want to do is add an allow_if_match to openstack.resource.Resource (kind of like allow_create / allow_patch etc) ... then put some code in openstack.resource.Resource._prepare_request to add the if-match header to the headers dict if there is an if_match parameter and if allow_if_match is true ... which is then going to probably involve some annoying plumbing to allow people | |
| 17:14:20 | mordred | to pass if_match to resource.commit() and get it all the way to _prepare_request | |
| 17:14:59 | mordred | (as well as to openstack.proxy.Proxy._update) | |
| 17:15:12 | mordred | gtema: ^^ does that sound sane to you? | |
| 17:15:32 | mordred | because I agree - you don't want an if-match property on the resource - it's not actually a part of the resource | |
| 17:15:33 | gtema | lemme read quickly | |
| 17:15:57 | mordred | gtema: https://review.opendev.org/710030 has the rest of the context | |
| 17:16:19 | dulek | That sounds sane, sure, but where in prepare_request() would I have the value of user's if-match? | |
| 17:16:28 | dulek | Seems like it doesn't get any user input at the moment. | |
| 17:16:33 | dulek | Though I guess it could? | |
| 17:16:56 | dulek | Okay, I see. | |
| 17:17:19 | dulek | So the good old plumbing is the correct way. :) | |
| 17:18:03 | mordred | yeah - I think we'd want them to call either my_network_resource.commit(conn.network, if_match='revision=3') - or conn.update_network(foo='bar', if_match='revision=3') | |
| 17:18:24 | mordred | we could get fancier and make our if_match take a dict instead of a strict and construct the string for them | |
| 17:18:39 | gtema | we don't expect if-match to ever be used outside of network, right? | |
| 17:18:47 | dulek | gtema: In such form - no. | |
| 17:19:21 | dulek | mordred: I'd probably prefer conn.update_network(foo='bar', if_match_rev=3). I don't think in Neutron you're allowed to use other property names anyway. | |
| 17:20:25 | gtema | we can then do the old way with _base resource in the network service, not to have changes in real openstack.Resource | |
| 17:20:29 | mordred | hrm. if it's only ever revision and we can confirm that, yeah - i'd prefer that - or even just "if_revision" | |
| 17:20:44 | mordred | gtema: yeah. | |
| 17:21:44 | mordred | ok. yeah - seems to just be revision: https://docs.openstack.org/api-ref/network/v2/#revisions | |
| 17:22:16 | mordred | we might want to check to see if the neutron supports the revision-if-match extension too - but we can probably skip that for v1 | |
| 17:22:30 | mordred | so I'd argue for "if_revision" or something else clean like that | |
| 17:23:14 | mordred | gtema: it's more widely used | |
| 17:23:32 | gtema | okay then | |
| 17:23:39 | gtema | never noticed so far | |
| 17:23:46 | openstackgerrit | Merged openstack/openstacksdk master: Adding basic implementation for Accelerator(Cyborg) https://review.opendev.org/679914 | |
| 17:24:08 | mordred | https://docs.openstack.org/swift/pike/overview_encryption.html | |
| 17:24:15 | dulek | mordred: Yeah, but now neutron-specific changes in base resource? Because that'd be really neutron-specific, it seems. | |