| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-11-01 | |||
| 13:54:07 | dansmith | which would make it easier to experiment with that on a running system when there's an issue like this | |
| 13:54:42 | dansmith | we could hard-code the ones we know are important, but others could be converted by virtue of being mentioned | |
| 13:54:50 | dansmith | I know, it's a little crazy, but.. | |
| 13:55:39 | mriedem | seems a bit fickle, meaning typos would be easy to screw that up, or someone renaming a method, though i doubt that ever happens | |
| 13:55:43 | sean-k-mooney | how would we do that. add a decorator that inspected the config on invocation and convert them to a long rpc if found or something like that | |
| 13:55:54 | mriedem | you'd just check if the method name is in the configured list | |
| 13:56:08 | mriedem | if 'migrate_server' in CONF.conductor.long_rpc_timeout_calls | |
| 13:56:12 | mriedem | is one way | |
| 13:56:21 | dansmith | mriedem: yep all true | |
| 13:56:45 | sean-k-mooney | right i was just wonderign would htis be spread in the code or could we centralise it in one mentod that they all uses | |
| 13:57:09 | mriedem | i think that would be more useful before we had long_rpc_timeout where you otherwise just had to configure the global rpc_response_timeout for everything if you had one slow thing, like select_destinations | |
| 13:57:22 | mriedem | i'm sure vio deals with that | |
| 13:57:33 | mriedem | now having said that, | |
| 13:57:46 | mriedem | i think we know it's an issue for some object indirection api db queries on big lists | |
| 13:58:21 | mriedem | e.g. https://review.opendev.org/#/c/633042/ | |
| 13:58:48 | mriedem | but that would also be harder to configure properly i think | |
| 13:59:02 | dansmith | we should make the indirection methods use it always anyway probably | |
| 13:59:23 | mriedem | long_rpc_timeout? i thought about it when investigating that bug | |
| 13:59:30 | dansmith | yes | |
| 14:00:51 | sean-k-mooney | what would be the cost of makeing long_rpc the default out of interest. i assume there is a reason for being selectiv beyond the fact many rpc methods dont strictly need it | |
| 14:01:27 | dansmith | it increases rabbit load a little bit | |
| 14:02:02 | sean-k-mooney | ok im just wondering how much of a wack a mole problem this is | |
| 14:02:28 | mriedem | for object methods, could we distinguish between an object list query and just a single object query? | |
| 14:02:37 | dansmith | it's exactly a whack-a-mole problem, which is why making it config-driven so people can whack said moles without code changes might be reasonable :) | |
| 14:02:47 | dansmith | mriedem: yes | |
| 14:04:17 | dansmith | mriedem: if you're requesting so many objects that you can't get it done in under the default limit, there are probably other issues, and if it's a load thing, then small queries can be delayed behind big ones, so.. I dunno how meaningful that distinction is | |
| 14:05:34 | mriedem | yeah. at what point do we let someone configure themselves to wait too long rather than solve their bigger load problems by scaling out/up their control plane. | |
| 14:05:48 | mriedem | i guess that answer probably comes down to their SLAs | |
| 14:05:56 | mriedem | or whatever their performance testing thresholds are | |
| 14:06:14 | dansmith | yeah, I'm not really suggesting we let them configure long rpc calls externally to solve problems, | |
| 14:06:30 | dansmith | more to experiment and report (or even just for our own experimentation in devstack) | |
| 14:06:37 | dansmith | not that it's really easier for us I guess | |
| 14:06:41 | dansmith | anyway, it was just a thought | |
| 14:15:48 | Sundar | mriedem, sean-k-mooney, dansmith: What would you like to see, either in the PTG or in terms of spec/patches, to move Cyborg forward? | |
| 14:17:53 | dansmith | Sundar: I'm not going to be at the ptg | |
| 14:20:35 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use long_rpc_timeout in conductor migrate_server RPC API call https://review.opendev.org/692550 | |
| 14:23:06 | Sundar | dansmith: I see. Would you like to see any changes in https://review.opendev.org/#/q/status:open+project:openstack/nova+bp/nova-cyborg-interaction ? Just checking to see what it takes to merge this patch series. | |
| 14:23:39 | dansmith | Sundar: reviews. doesn't look like it's had much of any, and the little it has had seems pretty un-diverse | |
| 14:23:51 | dansmith | which I know is why you're asking, just saying | |
| 14:24:29 | dansmith | I popped up one to see and it looks like it's in merge conflict and still has stuff like random whitespace damage in it (I'll comment in a sec), but that kind of stuff doesn't make it look like it's ready to go, fwiw | |
| 14:26:11 | Sundar | The conflicts happened recently. The series has passed Zuul checks for the most par and has been ready to go since Train merged. | |
| 14:26:50 | Sundar | Please ignore the conflicts for now and focus on the content | |
| 14:28:19 | mriedem | is there functional testing anywhere in the series? | |
| 14:28:23 | mriedem | i'm not even talking tempest, | |
| 14:28:32 | mriedem | but like creating a cyborg fixture in nova like we have for cinder and neutron, | |
| 14:28:52 | mriedem | so that we can start writing functional in-tree tests that use that fixture and at least exercise the runtime code in nova, despite using a fixture for the cyborg apis | |
| 14:29:14 | mriedem | i didn't ask/expect anyone to review my cross-cell resize series until i had some solid functional testing of at least the happy paths, | |
| 14:29:28 | mriedem | i don't think i even started asking for reviews until i had a multi-cell gate job working | |
| 14:29:58 | mriedem | right now this all looks like unit tests | |
| 14:30:05 | dansmith | mriedem: please ignore the conflicts and lack of testing and focus on the content | |
| 14:30:24 | mriedem | the next closest thing to the cyborg stuff is probably gibi's qos ports series | |
| 14:30:36 | mriedem | and he did a lot of functional test for those b/c of the complicated nature of dealing with nested provider allocations | |
| 14:31:01 | mriedem | so maybe you could build on some of that while also working on a beginning of a cyborg fixture for functional testing - at least the basic happy path, | |
| 14:31:27 | mriedem | create a server with arqs and then delete the server and verify the arqs are deleted/unbound whatever happens to those | |
| 14:31:42 | mriedem | the fixture would obviously have to track the state of the arqs associated with a server | |
| 14:31:53 | mriedem | which we do in the NeutronFixture for ports and volumes in the CinderFixture | |
| 14:32:18 | Sundar | mriedem: Looking at the discussion we had at the Cyborg/Nova cross-project at Denver PTG: https://etherpad.openstack.org/p/ptg-train-xproj-nova-cyborg | |
| 14:33:17 | Sundar | The criterion for integration was said to be upstream tempest CI with a fake driver. We got the fake driver done, and tempest can create/delete VMs wtth that fake driver now | |
| 14:34:20 | dansmith | Sundar: yeah, that's necessary too, | |
| 14:34:47 | dansmith | Sundar: but just about anything with tentacles running from API to the deepest depths of the compute service has serious functional testing | |
| 14:35:47 | Sundar | mriedem, dansmith: Just to be clear, you are talking about extending today's Nova functional testing, right? Not gabbi? | |
| 14:35:56 | mriedem | nova doesn't use gabbi | |
| 14:35:57 | mriedem | placement does | |
| 14:36:50 | mriedem | Sundar: some solid examples are what gibi did with qos port functional testing, for which this is the parent class https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L5565 | |
| 14:37:13 | mriedem | so here is an example https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L5996 | |
| 14:37:22 | mriedem | "Tests creating a server with a pre-existing port that has a resource request for a QoS minimum bandwidth policy." | |
| 14:37:54 | mriedem | we have other tests that interact with cinder https://github.com/openstack/nova/blob/master/nova/tests/functional/test_boot_from_volume.py | |
| 14:38:03 | mriedem | well, usinga CinderFixture | |
| 14:38:26 | mriedem | https://github.com/openstack/nova/blob/master/nova/tests/functional/test_multiattach.py | |
| 14:40:59 | Sundar | mriedem: Thanks, will look at it and follow up. | |
| 14:44:07 | mriedem | i traced the instance create/delete through the conductor and compute logs for the test_server_basic_ops tempest test and it looked fine, i saw the creating/waiting/deleting arqs stuff, no weird errors in the logs, so that's all good to see | |
| 14:44:40 | mriedem | once the series is rebased, if tests are passing and it's ready for review, put it into a runway slot | |
| 14:45:55 | mriedem | Sundar: ^ | |
| 14:47:20 | Sundar | mriedem: Thanks, will do. | |
| 14:48:02 | mriedem | Sundar: this is going to require a microversion bump https://review.opendev.org/#/c/631245/36/nova/api/openstack/compute/schemas/server_external_events.py@36 | |
| 14:48:09 | mriedem | you can't change schema on a versioned api without a new microversion | |
| 14:49:37 | dansmith | this does not look "ready to go since train" to me | |
| 14:49:49 | dansmith | there are things used in early patches that aren't added until later patches | |
| 14:50:12 | dansmith | and presumably very large test (even unit) gaps to allow that to happen | |
| 14:53:14 | Sundar | dansmith: "used in early patches that aren't added until later patches" -- What are you referring to? | |
| 14:53:30 | dansmith | Sundar: the event change. I left comments | |
| 14:54:09 | mriedem | Sundar: https://review.opendev.org/#/c/631244/43/nova/compute/manager.py@2609 | |
| 14:54:43 | mriedem | Sundar: note that nova still generally tries to subscribe to a philosophy of anything we merge today can be in production today, and people can CD nova | |
| 14:55:21 | mriedem | even if that's not the reality of how the vast majority of openstack consumers consume openstack, it's a development philosophy to try and avoid merging broken code | |
| 14:57:53 | mriedem | having said that i can't easily find anything about that in https://docs.openstack.org/nova/latest/contributor/ but i know it comes up every so often, and not all projects in openstack subscribe to the same philosophy, e.g. if it's all working by RC1 whatever, let it ride | |
| 14:59:36 | dansmith | probably because it's core openstack philosophy than not everyone has adhered to | |
| 14:59:40 | mriedem | closest is, https://docs.openstack.org/nova/latest/contributor/process.html#smooth-upgrades "upgrade from any commit, to any future commit, within the same major release" | |
| 14:59:42 | dansmith | is, or was | |
| 14:59:55 | mriedem | dansmith: i'm just not sure it's clearly documented anywhere | |
| 14:59:58 | mriedem | and probably should be | |
| 15:00:20 | mriedem | like something simple in https://docs.openstack.org/nova/latest/contributor/policies.html | |
| 15:01:00 | dansmith | sounds like a good thing to do tome | |
| 15:01:18 | Sundar | I have generally thought of the patch series as a unified whole. Merging individual parts will not give you any useful functionality. The base patch has a procedural -2, so that the whole series will merge together. | |
| 15:01:40 | dansmith | it's fine to not have incremental functionality, | |
| 15:01:46 | Sundar | That said, I don't believe in merging 'broken patches' either | |
| 15:01:52 | dansmith | it's not fine to merge things that use something before the thing is available | |
| 15:02:12 | dansmith | and we can't arrange for the whole series to merge as a whole, | |
| 15:02:34 | dansmith | which means the -2 holds things until all the pieces are ready, but they still have to be mergeable in isolation, because they will | |
| 15:02:54 | mriedem | especially since it takes about 3 days of rechecks to merge anything in nova right now | |
| 15:03:08 | dansmith | three days? please | |