| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-05 | |||
| 13:43:21 | cdent | I can’t decide. I’m trying to talk out loud to see if anything reasonable leaks out | |
| 13:43:26 | jaypipes | cdent: the meaning of "tree=X" is "get the root provider UUID of X and return all providers in that tree" | |
| 13:43:56 | jaypipes | cdent: so yeah, "limit to this provider's tree" is what the filter says. | |
| 13:44:16 | jaypipes | cdent: just want to be clear that "X" doesn't need to be the root provider UUID. | |
| 13:44:34 | jaypipes | cdent: it can be any old resource provider UUID. we look up that provider's root UUID. | |
| 13:44:54 | cdent | and if some other parameter (like resources) is present, and X isn’t in the resource satisfyng rps, no resource, right? | |
| 13:45:05 | cdent | s/no resource/no results/ | |
| 13:45:05 | jaypipes | correct | |
| 13:45:09 | efried_cya_wed | I was thinking ?tree=X&resources=Y would mean, "find me only the providers from within tree X that have resources Y" | |
| 13:45:19 | cdent | efried_cya_wed: it is not wed, go away | |
| 13:45:23 | efried_cya_wed | I.e. explicitly *not* the whole tree. | |
| 13:45:24 | jaypipes | efried_cya_wed: that is precisely what it means. | |
| 13:45:46 | jaypipes | efried_cya_wed: filters are "ANDed" together... | |
| 13:46:12 | cdent | I reckon in_tree is better | |
| 13:46:15 | cdent | but not hugely so | |
| 13:46:54 | alex_xu | cdent: X isn't in the resource satisfying rps, there may have result, for the case, the other rps match the resource in the tree | |
| 13:48:10 | jaypipes | alex_xu: yes, that's true. if X is a grandchild and Y is a child, and Y has all the resources needed, then Y would be returned, yes. | |
| 13:48:45 | alex_xu | jaypipes: yea | |
| 13:50:00 | jaypipes | alex_xu: we could call the filter 木 :) | |
| 13:50:12 | cdent | so a) in_tree is beginning to sound better to me, b) what’s the use case? when does a client want to do this? | |
| 13:51:31 | jaypipes | cdent: this is primarily going to be called by the scheduler report client's get_providers_in_tree() method which will populate a ProviderTree structure that is passed to the virt driver to populate | |
| 13:51:45 | alex_xu | jaypipes: you mean tree? it should be 树，木 is wood :) | |
| 13:51:59 | cdent | jaypipes: so in that case only the tree param is used, yes? | |
| 13:52:12 | jaypipes | alex_xu: crap! there's like 15 symbols that are "tree" in Google translate ;) | |
| 13:52:18 | alex_xu | haha | |
| 13:53:15 | jaypipes | cdent: yeah, in that case, only the tree filter is used | |
| 13:53:40 | alex_xu | the chinese version 'GET /资源_提供者?树=...&资源=...' | |
| 13:59:23 | alex_xu | jaypipes: I guess cdent is asking the use case of tree+resources | |
| 13:59:53 | cdent | not really. I agree that if we have filters, they should all be allowed and all should be and-ed | |
| 13:59:59 | cdent | I also agree that some combinations are weird | |
| 14:00:11 | cdent | but as long as we are and-ing correctly it is okay | |
| 14:00:28 | cdent | I don’t want us to be saying that some filter combinations are disallowed | |
| 14:00:54 | efried_cya_wed | ++ | |
| 14:01:21 | efried_cya_wed | Realistically, there are combinations consumers won't use because they don't make any sense. And that should be fine. | |
| 14:02:00 | alex_xu | cdent: +1 | |
| 14:17:03 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: [placement] Fix foreign key constraint error https://review.openstack.org/525620 | |
| 14:20:48 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 14:20:49 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: libvirt: Allow multiple volume attachments https://review.openstack.org/267587 | |
| 14:20:49 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Add a new check to volume attach https://review.openstack.org/525622 | |
| 14:21:54 | ildikov | mriedem: first attempt ^^ | |
| 14:22:36 | mriedem | ack | |
| 14:22:51 | ildikov | mriedem: I might've bumped the service version too early... :/ | |
| 14:23:53 | mriedem | yup | |
| 14:23:55 | mriedem | https://review.openstack.org/#/c/525622/1/nova/objects/service.py shouldn't be in there | |
| 14:24:15 | ildikov | yeah, I realized 5 minutes ago... | |
| 14:24:18 | mriedem | what is this? https://review.openstack.org/#/c/525622/1/nova/volume/cinder.py | |
| 14:24:36 | ildikov | checking the cinder microversion | |
| 14:24:56 | ildikov | or well, making it possible to do so | |
| 14:25:14 | ildikov | independently from the attachment_* calls | |
| 14:25:26 | mriedem | i don't think we want/need that in this patch, | |
| 14:25:37 | mriedem | what i was thinking was in the change that introduces _check_volume_already_attached_to_instance, | |
| 14:25:46 | mriedem | we'd just implement the usage of that in swap_volume, | |
| 14:25:56 | mriedem | because in swap volume we know if we're doing old or new flow based on the bdm.attachment_id, | |
| 14:25:59 | mriedem | in attach_volume we don't | |
| 14:26:10 | mriedem | so we need to leave the attach_volume changes for the last patch that adds the new flow | |
| 14:27:24 | ildikov | ah, ok | |
| 14:27:46 | ildikov | so you wanted to split out like 10 lines of code change? | |
| 14:28:07 | mriedem | it's more than that | |
| 14:28:17 | mriedem | it's the new method, plus the usage in swap_volume, plus tests | |
| 14:28:18 | ildikov | ok, 20 | |
| 14:28:51 | mriedem | ok - just leave it all in a 2K LOC change and we won't merge any of it if that's what you want | |
| 14:29:09 | ildikov | Jesus, Mary, St Joseph and the camel | |
| 14:29:19 | ildikov | sigh, no, I'll go and start over | |
| 14:29:52 | mriedem | i'm trying to help you split the things out that can be split out to make the main end patch more manageable for reviewers, | |
| 14:29:58 | mriedem | if we don't want to do that, then i'll give up | |
| 14:31:16 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Fix ValueError if invalid max_rows passed to db purge https://review.openstack.org/525628 | |
| 14:31:16 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Fix TypeError in nova-manage db archive_deleted_rows https://review.openstack.org/525629 | |
| 14:31:46 | ildikov | no, don't, I'm sorry, flying freaks me out, so I'm a few levels up regarding stress right now | |
| 14:32:56 | ildikov | and I've just uploaded the 167th revision which gives my stomach an extra bump... :/ :) | |
| 14:33:07 | ildikov | will ping you when I have a next version | |
| 14:33:25 | mriedem | ok | |
| 14:38:56 | sambetts | jaypipes: Is there a bug in Nova tracking the placement race condition we identified at the PTG between nova releasing the allocation and the ironic virt driver setting the number of resources available to zero? | |
| 14:40:06 | jaypipes | sambetts: not sure I follow you... | |
| 14:42:43 | sambetts | jaypipes: the race condition where on "nova delete" of an instance the allocation in placement is released so the node becomes free again, but its not actually free because Ironic is cleaning the node, so we set the resources to zero but for a brief period of time the node in placement can be reallocated because the resources are updated in a timed loop | |
| 14:44:34 | jaypipes | sambetts: but the node is not "available" according to the Ironic virt driver when it's being cleaned and therefore will not appear to the scheduler as passing the compute filter. | |
| 14:46:10 | sambetts | jaypipes: its only not avaiable because we set the avaiable resources for that node to zero, but the avaiable resources isn't refreshed instantly after an instance is deleted, but the allocation in placement is freed | |
| 14:46:45 | sambetts | so the node in placement can be reallocated until the resource tracker updates the avaiable resources | |
| 14:47:25 | jaypipes | sambetts: the allocation is deleted properly (the instance is no longer consuming resources on that node). it is the node itself that is marked as not available and therefore won't be scheduled to. | |
| 14:49:35 | sambetts | jaypipes: I'm not sure what you mean the node is marked as not avaiable, as far as I'm aware that happens by setting the resources to zero and then that causes the code to remove it from placement so it can't be scheduled too, is that what you are refering too | |
| 14:49:45 | sambetts | ? | |
| 14:49:47 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Add regression test for rebuilding a volume-backed server https://review.openstack.org/525632 | |
| 14:49:48 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Only query BDMs once in API during rebuild https://review.openstack.org/525633 | |
| 14:49:48 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Get original image_id from volume for volume-backed instance rebuild https://review.openstack.org/525634 | |
| 14:51:19 | jaypipes | sambetts: I'm referring to the scheduler, not placement. the scheduler checks to see whether a compute node/service is up and available to take requests before it attempts to schedule an instance to it. that check will return False for an Ironic node that is cleaning state. | |
| 14:51:23 | openstackgerrit | Merged openstack/python-novaclient master: Updated from global requirements https://review.openstack.org/525399 | |
| 14:52:25 | sambetts | jaypipes: is that new? I've never heard of that before | |
| 14:52:54 | jaypipes | sambetts: so placement might return that Ironic node to the scheduler as having space (now that the allocation was deleted), but the scheduler won't pick it until the node is available. | |
| 14:53:06 | jaypipes | sambetts: no, that's been like that since the beginning. | |
| 14:54:06 | jaypipes | sambetts: https://github.com/openstack/nova/blob/master/nova/scheduler/driver.py#L55-L61 | |
| 14:54:39 | jaypipes | though I'm looking at that now and seeing it's referring to the service (i.e. the nova-compute), not the baremetal node | |
| 14:54:42 | jaypipes | ffs | |
| 14:54:53 | jaypipes | this is why we can't ever have nice code... | |
| 14:55:09 | sambetts | :/ never seen anything like that in our driver, its all done based on resources | |
| 14:55:13 | sambetts | yeah :/ | |
| 14:57:07 | jaypipes | sambetts: well, there's this patch which should at least help with the Ironic situation: https://review.openstack.org/#/c/513526/ | |
| 14:57:41 | jaypipes | sambetts: that randomizes the returned results from placement so that (as is the case with Ironic) you won't always get back the same top node. | |
| 14:59:18 | sambetts | that'll certainly help, the other thing I think we suggested at the PTG was that we somehow instead of the resources getting marked as zero and then removed from placement all the time, we use the reserved field (although I'm still not sure that solves the race) | |
| 15:01:58 | sambetts | the race occurs because the resource tracker is async from the allocation getting deleted, so I'm sure what the right thing to do is there, unless we can force an refresh of that as soon as the resources are freed but then there is still a small period of time while the resource tracker runs | |
| 15:03:01 | sambetts | someone mentioned at the PTG that there are some hypervisors that have the same behaviour as this where even though an allocation has been deleted the resources aren't actually available for use yet | |
| 15:03:31 | sambetts | I'm trying to dig up the notes from the PTG session on it | |