Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-02-17
10:19:02 sean-k-mooney ya i dont really want to hold up the review on the subquery vs join approch escpially since i dont have performacne data one way or another
10:19:28 gibi sean-k-mooney: do you have any other comment that I should fix before FF?
10:19:39 sean-k-mooney so i woudl prefer to take the "get it working; then make it fast" approch provided there is no large regression
10:19:39 gibi I'm happy to if there is anythign
10:20:01 sean-k-mooney am not so far i have not got to the end of the series yet
10:20:13 sean-k-mooney but so far things look pretty good
10:21:06 gibi sean-k-mooney: thanks. let me know if I need to fix up thing quickly :)
10:21:12 gibi and thanks for the review
10:22:11 sean-k-mooney huh i didnt know that tuple unpacking work with dictionaries
10:22:16 sean-k-mooney https://review.opendev.org/c/openstack/placement/+/826491/8/placement/handlers/resource_provider.py
10:22:52 sean-k-mooney i understand how it could i guess i just tought that would be a syntax error
10:24:14 gibi yeah that was a nice touch from the python language that you can pack out into a dict
10:24:22 gibi into a dict value
10:24:28 sean-k-mooney gibi: its out of scope of the current spec but do you want to eventually support any_traits for root_required by the way
10:24:57 gibi I have no need for it right now, but if there is some use case from others then I can do it, most of the infra is in place for it
10:25:25 gibi probably the DB query needs some touching and the API needs to call the new trait parsing code
10:26:00 sean-k-mooney ok do we use root_required in nova today. i dont think so but perhaps we do
10:27:44 sean-k-mooney im not sure i have a usecase for any traits with it currently either but your serise will serve as a template for adding it if we need it in the future anyway
10:29:00 gibi sean-k-mooney: require_image_type_support, accelerators_filter and remote_managed_ports_filter prefilters are using it in nova today
10:29:17 gibi it is mosly to filter for compute capabilities
10:29:30 gibi and for that the simple AND trait list is enough
10:29:32 sean-k-mooney really i tought those were just in the unnamed group
10:29:48 gibi they could be there too
10:29:58 gibi both works today as we always allocate from the root
10:30:22 gibi the code is a bit simpler with root_required as you don't need to find the unnamed group
10:30:22 sean-k-mooney ya i just was not aware we converted to useing root_requried for that
10:30:35 sean-k-mooney ya
10:35:46 sean-k-mooney gibi: which patch are you adding the 1.39 micorversion is it the last patch
10:35:54 gibi yes
10:35:56 sean-k-mooney ah yes it is
10:36:19 sean-k-mooney ok i was just looking at the func test changes and wondering why you only had the negitive tests for 1.38
10:36:28 sean-k-mooney ok that makes sense
11:16:45 gibi yuval: left comments in https://review.opendev.org/c/openstack/nova/+/821606
11:19:54 yuval Thanks
11:32:45 sean-k-mooney yuval: how much work woudl it be to entirely factor out the monitor thread and "message queue" out of the nova code
11:33:12 sean-k-mooney i really dont like the idea of os-brick or the volume driver spawnign a background thread
11:33:46 sean-k-mooney im pretty sure i have objected to adding that to os-brick on the mailing list in the past
11:34:26 gibi chateaulav: I have an idea what is wrong with the object compatibility code still in https://review.opendev.org/c/openstack/nova/+/828369
11:34:52 gibi chateaulav: left a comment
11:35:26 sean-k-mooney hum we likely can move the call to super to the end
11:36:33 sean-k-mooney gibi: actully looking at the version im a littel confused why was a version bump done for 1.18 and 1.19
11:36:59 gibi sean-k-mooney: I think remotable methods need a bump
11:36:59 sean-k-mooney i have not looked at the patches that added those but adding a new method does not require an object version bump
11:37:10 sean-k-mooney ah
11:37:16 sean-k-mooney they are not jsut local
11:37:21 sean-k-mooney perhaps
11:38:00 gibi about the super call, I think yes we can move even way down
11:38:21 sean-k-mooney i would prefer not to put it in the midel of the compat ifs
11:38:47 sean-k-mooney so either have it first or after all of them ideally
11:40:56 yuval sean-k-mooney: can you add your comment to the patchset I discuss it with my colleagues
11:42:11 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/821606/8/nova/virt/libvirt/volume/lightos.py#59
11:46:13 sean-k-mooney yuval: this is the feedback i left on the cinder spec over a year ago https://review.opendev.org/c/openstack/cinder-specs/+/766732/12/specs/wallaby/nvme-agent.rst#73
11:53:18 sean-k-mooney yuval: os-bricks executor.Thread is just a monkey patched green thread so if you are doing long running work that never yeild it will block nova
11:56:44 chateaulav gibi: sounds good, I'll take a look as soon as get in in about an hour
11:56:52 gibi chateaulav: ack
11:58:36 sean-k-mooney yuval: this for exampel will block nova until its finished processing it https://github.com/openstack/os-brick/blob/master/os_brick/initiator/connectors/lightos.py#L145-L162
11:58:59 sean-k-mooney so these sleeps https://github.com/openstack/os-brick/blob/master/os_brick/initiator/connectors/lightos.py#L174-L180
11:59:11 sean-k-mooney are the only thing that cause that to yeild back to nova
12:02:44 sean-k-mooney some of the operation in https://github.com/openstack/os-brick/blob/5902166149ba46da5f36eb352e62f3361269fb2c/os_brick/initiator/connectors/lightos.py#L107 also do io which will cause eventlet to yeild but in genrealy i dont think we want to allow that to run in a thread in the compute agent
12:04:24 sean-k-mooney lightos is not the only problematic driver that is now tryign to do this
12:05:30 sean-k-mooney but i really dong like this design parrent in os-brick and i think all montors/deamon threads shoudl be seperate processes or not part of os-brick at all
12:07:13 sean-k-mooney ... os-brick is doing this in several places.... that is really not good we should probaly make this a ptg topic
12:19:32 yuval ok, thanks for the feedback, we will think what our options
12:23:41 sean-k-mooney gibi: by the way nova still has root wrap solely for os-brick right. is there a reason we cant move this to os-brick?
12:23:43 sean-k-mooney https://github.com/openstack/nova/blob/master/etc/nova/rootwrap.d/compute.filters
12:25:36 sean-k-mooney gibi: looking at what os-brick is doing really make me think we shoudl not be loading it in the compute agent directly since its problematic in several ways
12:26:11 gibi I think only os-brick using rootwrap we converted the rest of privsep
12:26:16 gibi so I think we can move it
12:26:27 gibi but probably we need a volunteer
12:26:57 sean-k-mooney yep its just techdebth
12:26:57 gibi as of what os-brick does under the hood, I'm not that familiar with. I saw that iscsi connector also spawn threads so I assume that works other
12:27:12 gibi otherwise we would have failures
12:27:23 sean-k-mooney it works but they are green threads
12:27:27 sean-k-mooney not native threads
12:27:36 sean-k-mooney because we monkey patch and its running in the same process
12:27:50 sean-k-mooney os-brick does not use eventlets iteself
12:28:08 sean-k-mooney which is true for os-vif alos both both inherit it becasue they are imported into nova which does
12:28:46 gibi ahh you have a good point
12:28:58 gibi those just look like native threads but we monkey patched them
12:29:05 gibi but then I don't fully get the problem
12:29:21 gibi why don't they spawn greenlets freely?
12:29:25 gibi under the hood
12:29:52 sean-k-mooney gibi: simple since they are monkey patched if they dont yeild nova's event loop is blocked
12:30:11 gibi they have thight loops without io?
12:30:22 sean-k-mooney yes in some places
12:30:31 gibi ahh so they have some thight loops
12:30:41 gibi then those needs some careful yields
12:30:59 sean-k-mooney https://github.com/openstack/os-brick/blob/master/os_brick/initiator/connectors/lightos.py#L164-L182
12:31:38 sean-k-mooney so this yeild because monitor_db does some writes to a file and those sleeps
12:32:11 sean-k-mooney monitor_message_queue wont yeild unless the log satement casue it to happen
12:32:29 sean-k-mooney but it will terminate when the queue is empty
12:32:52 gibi I see
12:33:41 sean-k-mooney isci is potentally worse
12:33:43 sean-k-mooney https://github.com/openstack/os-brick/blob/master/os_brick/initiator/connectors/iscsi.py#L723-L780
12:34:04 sean-k-mooney it spawns a bunch of threds then joins in the multipath code
12:34:47 sean-k-mooney the join shoudl cause a yeild and those threads will be interleaved with nova
12:35:11 sean-k-mooney but this type of raw threadign is really not a good thing with eventlets in the mix
12:35:31 gibi jeah that seem a lot more complex. but still it works or we would have strange bugs
12:35:36 sean-k-mooney they also have sleeps https://github.com/openstack/os-brick/blob/master/os_brick/initiator/connectors/iscsi.py#L777
12:36:35 sean-k-mooney ya so that will do io an dthe sleep will yeild which the do on each iteration

Earlier   Later