| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-31 | |||
| 16:52:04 | melwitt | yeah, it would be surprising if it's not working but I admit I don't understand how it's working with a blanked out non-admin context | |
| 16:52:12 | dansmith | yeah | |
| 16:52:30 | dansmith | I think I would admin the eff out of that mah fah | |
| 16:52:38 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Use placement 1.28 in scheduler report client https://review.openstack.org/583667 | |
| 16:52:40 | melwitt | there are places we do the same thing in nova-manage too, and those work too I think. hm. | |
| 16:52:53 | melwitt | yeah, I'll change it to admin | |
| 16:53:30 | melwitt | ah, it's doing context.elevated() | |
| 16:53:35 | dansmith | lol | |
| 16:53:36 | dansmith | hah | |
| 16:53:36 | melwitt | I missed that | |
| 16:53:50 | dansmith | maybe just get_admin_context() then? :) | |
| 16:53:56 | melwitt | right at the instance list call | |
| 16:54:13 | melwitt | I'm not disagreeing with you, I'd rather have it be more obvious when it's created | |
| 16:54:29 | melwitt | i.e. make it admin | |
| 16:54:31 | dansmith | yeah, I'm just making fun of the round-about-ness causing confusing | |
| 16:54:33 | dansmith | *confusion | |
| 16:54:39 | melwitt | I just didn't get how this was working until I finally noticed the elevated() call | |
| 16:54:53 | melwitt | ah, yeah :) | |
| 16:56:47 | gibi | tetsuro: FYI, made a bit of progress in https://review.openstack.org/583667 and pushed it up. I don't have any other modification in progress locally, so you are free to change the series while I asleep | |
| 16:56:49 | melwitt | yay, get to keep part of mah patch | |
| 16:58:41 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Remove usage of migrateToURI{2} APIs https://review.openstack.org/567258 | |
| 16:58:48 | kashyap | sahid: gibi: Thanks, I've addressed the final nits ^ | |
| 16:59:35 | kashyap | melwitt: In your "copious free time", appreciate if you'll have a gander, as you've reviewed a past iteration | |
| 16:59:56 | melwitt | okay, in nova-manage we're using non-admin RequestContext but we call elevated() if we're going to query for instances | |
| 16:59:57 | kashyap | sahid: s/nit/required missing bit in the test/ (which is not a nit :-)). | |
| 17:00:07 | melwitt | kashyap: ack | |
| 17:00:47 | melwitt | which makes sense, only use admin-ness when you need it | |
| 17:02:27 | melwitt | it just throws me off a lot because everywhere else we use get_admin_context for similar things | |
| 17:08:50 | gibi | kashyap: thanks. I put back my +2 | |
| 17:14:44 | openstackgerrit | melanie witt proposed openstack/nova master: Don't overwrite greenthread-local context in host manager https://review.openstack.org/582332 | |
| 17:21:51 | dansmith | man, this poor guy keeps getting disconnected trying to tell us his story | |
| 17:26:30 | cdent | I'm more interested in the fentanyl | |
| 17:30:33 | cdent | thanks! | |
| 17:34:47 | mriedem | johnthetubaguy: you know this Stats object that we manage in the ResourceTracker per node which gets used in the scheduler? could that ever possibly be correct for ironic? | |
| 17:35:27 | mriedem | because RT.stats is a cumulative set of stats for all instances managed by the compute host, | |
| 17:35:31 | mriedem | and gets copied per node | |
| 17:39:45 | mriedem | although i thought before traits, people would use ComputeCapabilitiesFilter for ironic | |
| 17:39:49 | mriedem | and that relies on stats | |
| 17:39:57 | mriedem | jroll: ^? | |
| 17:40:35 | jroll | hm | |
| 17:41:29 | jroll | I thought stats was per node, not host | |
| 17:41:53 | mriedem | they are, | |
| 17:41:54 | mriedem | but see https://review.openstack.org/#/c/576099/2/nova/compute/resource_tracker.py@1130 | |
| 17:43:13 | jroll | mriedem: # purge old stats and init with anything passed in by the driver | |
| 17:43:28 | jroll | tells me it's handled, without digging too deep | |
| 17:43:53 | mriedem | oh i didn't see that | |
| 17:44:01 | jroll | line 606 | |
| 17:44:05 | mriedem | yeah | |
| 17:44:10 | mriedem | god this is terrible | |
| 17:44:14 | jroll | removing the copy seems bad for ironic though | |
| 17:44:26 | mriedem | yeah that was efried's point | |
| 17:44:35 | jroll | right, ok | |
| 17:52:21 | mriedem | even with that clear at the beginning, | |
| 17:52:28 | mriedem | i think we're not tracking stats properly per node | |
| 17:52:41 | mriedem | because we're looping over instances managed by this host, | |
| 17:52:47 | mriedem | and instances are 1:1 with nodes, | |
| 17:52:50 | mriedem | for ironic, | |
| 17:53:01 | mriedem | and then we do this *per instance*: | |
| 17:53:05 | mriedem | cn.stats = copy.deepcopy(self.stats) | |
| 17:53:05 | mriedem | self.stats.update_stats_for_instance(instance, is_removed_instance) | |
| 17:53:32 | mriedem | so over the course of let's say 10 instances, wouldn't the stats on all by the first node be wrong? | |
| 17:53:55 | mriedem | i.e. stats.num_instances > 1 for all but the first node | |
| 18:00:04 | dansmith | mriedem: we have one RT per node, thus one stats per node, yeah? | |
| 18:00:39 | mriedem | don't think so, | |
| 18:00:50 | mriedem | compute manager has a single RT which is managing multiple nodes | |
| 18:01:23 | mriedem | that's why we have ResourceTrack.compute_nodes | |
| 18:02:08 | mriedem | the problem is for vmware, a single host manages 1500 VMs or something | |
| 18:02:19 | dansmith | I always forget how this works, | |
| 18:02:22 | mriedem | and the RT does a deepcopy on the stats object per instance during the update_available_resource periodic | |
| 18:02:38 | dansmith | but I thought we did have multiple RTs but the single-rt bits of compute manager only work when they're 1:1 | |
| 18:03:21 | dansmith | I guess I'm wrong, | |
| 18:03:23 | mriedem | that might have been true before 2016 | |
| 18:03:29 | dansmith | oh | |
| 18:03:30 | mriedem | when jaypipes refactored all of the node management in the RT | |
| 18:03:33 | dansmith | ah | |
| 18:03:44 | dansmith | I see we call it with a node name | |
| 18:04:04 | mriedem | https://review.openstack.org/#/c/398473/ | |
| 18:04:14 | mriedem | ^ makes ComputeManager go from >1 RT to 1 RT | |
| 18:04:18 | dansmith | ah yeah | |
| 18:04:22 | dansmith | glad I'm not crazy | |
| 18:04:45 | mriedem | so, | |
| 18:04:49 | mriedem | i think i'm right, | |
| 18:04:58 | mriedem | in that self.stats would be cumulative for all nodes managed by this host | |
| 18:05:03 | mriedem | in the before times, it wouldn't, | |
| 18:05:06 | mriedem | because we'd have 1 RT per node | |
| 18:05:10 | mriedem | managed by the compute manager | |
| 18:07:20 | mriedem | hmm, i guess no major baremetal deployments have been affected since ocata, or aren't at ocata yet, or aren't using the ComputeCapabilitiesFilter | |
| 18:07:27 | mriedem | or they are affected but don't know it | |
| 18:07:44 | jroll | tripleo uses ComputeCapabilitiesFilter for everything, iirc | |
| 18:07:46 | dansmith | baremetal stats reporting has been wrong for a long time right? | |
| 18:07:55 | jroll | you'd think they'd have noticed by now | |
| 18:08:05 | mriedem | i guess it depends on what they are using it for, | |
| 18:08:19 | mriedem | HostState.num_instances, which is derived from stats.num_instances, would be wrong | |
| 18:08:42 | mriedem | but i think the ironic driver passes through stats from the ironic node which goes into the stats object which then gets reported to the HostState for filtering | |
| 18:09:16 | mriedem | yeah https://github.com/openstack/nova/blob/master/nova/virt/ironic/driver.py#L330 | |
| 18:09:28 | mriedem | ironic node capabillities get proxied to the scheduler via the Stats object | |
| 18:12:34 | mriedem | jroll: i have to assume you guys at oath are using ComputeCapabilitiesFilter for ironic scheduling | |
| 18:12:38 | mriedem | or have you moved to traits? | |
| 18:12:48 | jroll | mriedem: we are not | |
| 18:13:01 | jroll | our flavors are basically ram/disk/cpu or model numbers | |