| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-25 | |||
| 15:24:34 | mriedem | b/c something isn't adding up | |
| 15:25:44 | mriedem | deleting a nova-compute service record doesn't delete the compute_nodes record, | |
| 15:26:07 | mriedem | but if you restart the nova-compute process, it's going to create a new services table record for the same host/binary/topic | |
| 15:26:15 | mriedem | the compute_nodes table entry has a service_id record in it | |
| 15:26:33 | mriedem | the resource tracker isn't going to create a new compute_nodes record because one already exists for the given host and nodename | |
| 15:26:43 | mriedem | but, do we ever update the compute_nodes.service_id record anywhere? | |
| 15:27:46 | openstackgerrit | Sean Dague proposed openstack/nova-specs master: oslo.middleware request_id spec https://review.openstack.org/468066 | |
| 15:27:52 | sdague | mriedem: ok, spec ^^^ | |
| 15:28:17 | mriedem | ok | |
| 15:30:19 | mriedem | jaypipes: do we actually not tie the compute nodes record to the nova-compute service anywhere? | |
| 15:31:31 | mriedem | ah looks like it's not, by design | |
| 15:31:34 | mriedem | since it's all deprecated | |
| 15:35:53 | mriedem | oh snap | |
| 15:36:06 | mriedem | alex_xu: we do soft delete the compute node record when the service is deleted, | |
| 15:36:09 | mriedem | if they are tied together, | |
| 15:36:15 | mriedem | which happens for very old compute nodes records | |
| 15:36:52 | mriedem | https://github.com/openstack/nova/blob/65402e1aeb770d077620f2935a0cb305474aa7a1/nova/db/sqlalchemy/api.py#L461 | |
| 15:37:15 | jaypipes | mriedem: we removed the direct linkage between the two on purpose | |
| 15:37:27 | mriedem | https://github.com/openstack/nova/blob/65402e1aeb770d077620f2935a0cb305474aa7a1/nova/db/sqlalchemy/api.py#L461 | |
| 15:37:28 | mriedem | yeah | |
| 15:37:37 | mriedem | the db api tests are still doing the very old style linkage | |
| 15:37:47 | mriedem | so testing using those for a modern setup is really confusing | |
| 15:46:15 | jaypipes | edleafe: k, sorry about that. ready now. | |
| 15:46:16 | jaypipes | hangout? | |
| 15:51:20 | mriedem | sdague: -1 is specifically for expanding on this https://review.openstack.org/#/c/468066/1/specs/pike/approved/oslo-middleware-request-id.rst@48 | |
| 15:51:23 | mriedem | otherwise +2 | |
| 15:52:01 | sdague | mriedem: so, it's not sufficient that the patch already has that in it? | |
| 15:52:12 | mriedem | the spec is a doc | |
| 15:52:22 | sdague | sure | |
| 15:52:24 | mriedem | published as a doc, people reading those later shouldn't have to dig through lp to find the patch | |
| 15:52:55 | edleafe | jaypipes: cdent: so you suggested passing the instance UUIDs in the reqspec | |
| 15:53:20 | edleafe | That was my original approach, but dansmith preferred the RPC parameter approach, so I changed it | |
| 15:53:38 | edleafe | jaypipes: see https://review.openstack.org/#/c/463623/ | |
| 15:53:45 | jaypipes | edleafe: right. | |
| 15:54:16 | jaypipes | edleafe: lemme walk through in my head whether the recent "continuing the automatic reschedule behaviour" thing affects my decision on that... | |
| 15:54:18 | cdent | edleafe: I don't really have a strong opinion. my natural inclination is parameters, but that's not been seasoned by years of nova-based RPC | |
| 15:55:07 | edleafe | jaypipes: cdent: my preference was reqspec (which is why I did it that way first), since it already had the pretty much useless 'instance_uuid' attribute | |
| 15:55:08 | openstackgerrit | Sean Dague proposed openstack/nova-specs master: oslo.middleware request_id spec https://review.openstack.org/468066 | |
| 15:55:13 | jaypipes | dansmith: can you, in one sentence, explain your preference to change the RPC interface vs. add a field to the reqspec object? | |
| 15:55:21 | sdague | mriedem: done | |
| 15:55:35 | jaypipes | dansmith: I want to make sure I understand you completely. | |
| 15:56:00 | dansmith | jaypipes: we don't need to store the uuids in the database, right? | |
| 15:56:22 | dansmith | we've had this conversation a couple times | |
| 15:56:44 | edleafe | dansmith: but there is already the 'instance_uuid', which we don't store either | |
| 15:57:46 | dansmith | edleafe: um, what? | |
| 15:57:53 | dansmith | already instance_uuid in the reqspec? | |
| 15:58:00 | edleafe | yup | |
| 15:58:33 | edleafe | dansmith: https://review.openstack.org/#/c/463623/5/nova/objects/request_spec.py@77 | |
| 15:58:38 | dansmith | what do you mean we don't store it | |
| 15:58:39 | dansmith | ? | |
| 15:58:49 | dansmith | ...right, I know it's in there | |
| 15:58:54 | edleafe | well, we don't *need* to store it | |
| 15:59:12 | edleafe | it's useless if num_instances > 1 | |
| 15:59:15 | dansmith | okay not sure what that has to do with not adding another thing we don't need to store, which could be a hundred uuids :) | |
| 15:59:31 | mriedem | fuel on the fire makes it better right? | |
| 15:59:50 | mriedem | at what point do we add kitchen_sink to request spec? | |
| 16:00:21 | edleafe | I understand the reqspec to be the vehicle for passing the requested info to the scheduler | |
| 16:00:28 | edleafe | the scheduler needs this info | |
| 16:01:28 | jaypipes | dansmith: so the reason I was advocating passing in all the instance uuids in the reqspec was because I wanted to get rid of the retry stuff and do all of that in the scheduler (and thus we'd need all those UUIDs). But now that that is off the table, I'm not sure it makes sense any more. | |
| 16:01:50 | dansmith | aye | |
| 16:01:52 | edleafe | jaypipes: it definitely does | |
| 16:02:22 | edleafe | if num_instances > 1, how will the scheduler claim the resources if it doesn't know the consumer_id? | |
| 16:02:38 | dansmith | the uuid list is only something that makes sense for a single call to the scheduler, AFAIK, so it shouldn't be stuffed into the database, just something we pass like anything else, IMHO | |
| 16:03:19 | jaypipes | edleafe: a fair point. | |
| 16:03:52 | jaypipes | edleafe: if we moved the handling of num_instances > 1 to the conductor, though... | |
| 16:04:23 | jaypipes | edleafe: we'd need more robust concurrent update handling in the conductor, then. | |
| 16:04:24 | edleafe | jaypipes: sp the conductor would make num_instances calls to the scheduler, instead of 1? | |
| 16:04:32 | jaypipes | edleafe: I'm spitballing. | |
| 16:04:49 | edleafe | We'd also have to run the host queries num_instances times instead of once | |
| 16:05:05 | jaypipes | edleafe: right. | |
| 16:05:06 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Exclude deleted service records when calling hypervisor statistics https://review.openstack.org/467220 | |
| 16:05:16 | edleafe | you're spitballing; I'm poking holes | |
| 16:05:19 | edleafe | :) | |
| 16:05:24 | mriedem | Kevin_Zheng: ^ i updated the commit message and re-wrote the test a bit | |
| 16:05:25 | jaypipes | edleafe: I'm thinking out loud. | |
| 16:05:29 | mriedem | with a bunch of comments to explain it | |
| 16:05:45 | edleafe | me too | |
| 16:05:58 | edleafe | I've thought about these a lot already, though :) | |
| 16:06:07 | sdague | mriedem: I'm confused about your comment on https://review.openstack.org/#/c/467995/ about implications for versioned objects | |
| 16:06:28 | sdague | maybe that's where we need dansmith poking | |
| 16:07:39 | openstackgerrit | Ed Leafe proposed openstack/nova master: Pass the number of hosts to return explicitly https://review.openstack.org/465767 | |
| 16:07:40 | openstackgerrit | Ed Leafe proposed openstack/nova master: Remove selecting host based on host_subset_size https://review.openstack.org/465174 | |
| 16:07:40 | openstackgerrit | Ed Leafe proposed openstack/nova master: Return multiple hosts from select_destinations https://review.openstack.org/465768 | |
| 16:08:01 | edleafe | jaypipes: ^^ The series is ready for review. I can switch the passing of UUIDs if needed | |
| 16:08:08 | jaypipes | edleafe, dansmith: for retries, do we ever retry the entire list of instances (in a num_instances > 1 case) or do we just retry the launch of one of them? | |
| 16:08:36 | edleafe | jaypipes: if we retry the whole list, that would be very dumb | |
| 16:08:52 | edleafe | since some could have already been successful | |
| 16:09:00 | jaypipes | edleafe: well, it all depends on how "atomic" we view the num_instances > 1 case, right? | |
| 16:10:05 | dansmith | jaypipes: we've lost scope of the others by the time we're retrying any of them | |
| 16:10:11 | dansmith | jaypipes: and they might've scattered across cells | |
| 16:10:23 | dansmith | jaypipes: as you've pointed out, the api implies atomicity, and we guarantee almost none | |
| 16:10:25 | mriedem | sdague: yes, that's my call for dan | |
| 16:11:01 | edleafe | the min_count and max_count were supposed to provide a go/no-go atomicity | |
| 16:12:54 | sdague | mriedem: ok, cool. Jenkins is currently a happy camper, so there is at least that | |
| 16:13:38 | jaypipes | dansmith, edleafe: ok, random idea here... what if we triggered claims in the scheduler only if num_instances == 1? at least for right now... and let the num_instances > 1 case be handled in the same wonky claim-on-compute way it is now? | |
| 16:13:56 | dansmith | oh please god no | |
| 16:14:03 | cdent | what would that gain? | |
| 16:14:12 | dansmith | jaypipes: nobody wants to claim on compute right? | |
| 16:14:31 | jaypipes | it would mean we would solve the 80% case now. and come up with a solution for the 20% case later. | |
| 16:14:34 | edleafe | jaypipes: yeah, that sounds terrible | |