| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-31 | |||
| 16:48:30 | dansmith | it's a pattern I recognize, but don't recognize the get_context() bit even though they're basically the same | |
| 16:48:42 | melwitt | oh, I see. yeah, it could be, but I wasn't thinking to change it from what it was before because I don't know if it was intentional | |
| 16:48:50 | dansmith | anyway, yes probably worth keeping that bit | |
| 16:49:01 | dansmith | yeah, I'm surprised it doesn't need to be admin because it's listing all instances right? | |
| 16:49:16 | dansmith | maybe it works because it has no project_id? | |
| 16:49:20 | dansmith | anyway, seems weird | |
| 16:49:40 | dansmith | those are clearly admin-y internal cache-fill ops, so I would think.. admin. | |
| 16:49:45 | melwitt | yeah, that's a good point. it would miss instances where project_id doesn't match, so maybe it misses instances | |
| 16:50:08 | dansmith | well, it's setting project_id=None, so presumably it's working, but weird | |
| 16:50:19 | melwitt | admin doesn't have to match project_id for instance get_all but non-admin does. I have learned that the hard way from func test writing | |
| 16:50:28 | dansmith | also, would it not make sense to make oslo_context not overwrite context in TLS by default if one is already set? | |
| 16:50:52 | melwitt | I dunno, tbh | |
| 16:50:55 | dansmith | melwitt: right, but surely this isn't so broken | |
| 16:51:03 | dansmith | that it's not collecting any instances right? | |
| 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 | |