| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-27 | |||
| 16:58:50 | johnthetubaguy | figleaf: at least I think it will | |
| 16:58:51 | openstackgerrit | Sarafraj Singh proposed openstack/nova: Adopts keystoneauth with glance client. https://review.openstack.org/412634 | |
| 16:58:55 | sfinucan | Interesting problem here - I've backported a bugfix that's exposed a latent Python 3 issue in the code https://review.openstack.org/#/c/425087 | |
| 16:59:11 | johnthetubaguy | figleaf: cdent: maybe a quick google hangout would make this easier to talk through? | |
| 16:59:14 | sfinucan | Wonder if I should fix the Python 3 issue or drop the test? | |
| 17:01:48 | figleaf | johnthetubaguy: maybe, but I'm switching context to follow the code through the different parts | |
| 17:02:16 | figleaf | johnthetubaguy: I can't keep it all straight while working on something else :) | |
| 17:02:23 | johnthetubaguy | figleaf: no worries, it took me a good while to trace all that back too | |
| 17:05:45 | superdan | sdague: I have to merge something in devstack to make grenade consider a new projects/XX_thing plugin? | |
| 17:06:32 | cdent | johnthetubaguy: will objects.InstanceList.get_by_host_and_node return ironic things? | |
| 17:06:54 | johnthetubaguy | cdent: I believe it does | |
| 17:07:22 | superdan | cdent: it'll return ironic instances yeah | |
| 17:07:56 | cdent | k, thanks johnthetubaguy and superdan, trying to properly trace the code (as figleaf said, is meandering...) | |
| 17:09:01 | superdan | sdague: oh, nm I think i see | |
| 17:12:01 | openstackgerrit | John Garbutt proposed openstack/nova: Scheduler calling the Placement API https://review.openstack.org/417961 | |
| 17:12:02 | openstackgerrit | John Garbutt proposed openstack/nova: Block starting compute unless placement conf is provided https://review.openstack.org/425806 | |
| 17:13:13 | mriedem | johnthetubaguy: ah i see you updated it :) | |
| 17:13:19 | mriedem | was just leaving comments, will finish that up on the older PS | |
| 17:13:46 | johnthetubaguy | mriedem: hopefully, that was a rebase, but I think it was superdan 's change that failed to merge, but never mind | |
| 17:15:57 | mriedem | johnthetubaguy: just posted comments in PS36 | |
| 17:16:37 | jroll | mriedem: sure why not :P | |
| 17:16:55 | johnthetubaguy | mriedem: I thought I had removed all that :S | |
| 17:17:50 | mriedem | johnthetubaguy: want to update the wording on https://review.openstack.org/#/c/425806/4/nova/exception.py ? | |
| 17:17:53 | mriedem | while you're at it | |
| 17:18:31 | johnthetubaguy | mriedem: aye | |
| 17:18:43 | figleaf | johnthetubaguy: cdent: so update_instance_allocation() should only be called for "old-style" allocations. create_provider_allocations() should be called for "new-style" (i.e., ironic with custom RC) | |
| 17:19:15 | superdan | mriedem: johnthetubaguy I'm going to avoid rechecking the nova scheduler patch until I have reasonable faith in the refactor to move placement into its own top-level | |
| 17:19:16 | superdan | just FYI | |
| 17:19:43 | jroll | More than one endpoint exists with the name 'placement'. <- not cool, devstack, not cool | |
| 17:19:56 | figleaf | johnthetubaguy: cdent: what it looks like is that we need to make sure that the update_instance_allocation() calls are all checked to make sure that they aren't called for new ironic nodes | |
| 17:20:19 | mriedem | johnthetubaguy: i'll go through PS38 too | |
| 17:20:36 | johnthetubaguy | figleaf: but there are no new calls right now, so surely we need someone to claim the new resource class? | |
| 17:21:36 | figleaf | johnthetubaguy: I only see two places where update_instance_allocations() is called in the RT. | |
| 17:21:44 | johnthetubaguy | figleaf: I would rather update_instance_allocation() was able to know from the instance what resources it should be claiming, and it just claims the correct ones, for now thats a bit hard coded I guess, but eventually its the same logic the scheduler has to do when requesting the correct resource class | |
| 17:22:24 | figleaf | johnthetubaguy: yeah, this is all transitional code | |
| 17:22:59 | figleaf | johnthetubaguy: we wanted to get ironic handled before we are able to handle all types of RPs | |
| 17:23:21 | figleaf | johnthetubaguy: can you think of a test that would fail now with ironic? | |
| 17:23:42 | johnthetubaguy | figleaf: add resource class on ironic node, boot instance that will land there, resource claim fails | |
| 17:23:50 | johnthetubaguy | figleaf: at least, thats the way it looks to me | |
| 17:23:56 | cdent | figleaf, johnthetubaguy my read of the code in the rt ironic inventory patch is that johnthetubaguy is right: it is possible to reache update_instance_allocation with a compute node and an instance and depending on what "compute_node" is, the allocations will either fail, or apply to the nova-compute node, incorrectly | |
| 17:24:19 | cdent | because the data at that point is about resource usages (vcpu etc) | |
| 17:24:49 | johnthetubaguy | yeah, instead of allocating for vcpu, it should be for the custom resource class, I think | |
| 17:25:21 | johnthetubaguy | oh balls... | |
| 17:25:21 | cdent | which should mean that the ironic "instance" is not include in the loop that is calling update_instance_allocation | |
| 17:25:27 | cdent | because it isn't really an instance | |
| 17:25:36 | cdent | (in that context) | |
| 17:25:45 | johnthetubaguy | if we merge the code to query placement, we will never land on an iroinc node in the first place, unless it is reporting VCPU resources | |
| 17:26:02 | figleaf | johnthetubaguy: that's a known issue | |
| 17:26:11 | cdent | "oh balls" means we've passed the "bugger" state of affairs | |
| 17:26:23 | johnthetubaguy | cdent: heh, ack | |
| 17:26:25 | figleaf | johnthetubaguy: which is why ironic has to keep reporting vm-type resources until Pike | |
| 17:26:45 | johnthetubaguy | figleaf: but this code doesn't do that, I thought | |
| 17:27:00 | cdent | brb | |
| 17:27:21 | figleaf | johnthetubaguy: digging into the claim stuff - haven't looked at that in a while | |
| 17:28:43 | johnthetubaguy | figleaf: actually, I am changing my mind on the fix here, I think its probably just that we should still be reporting VCPUs, we can't drop those till we schedule based on the new resources | |
| 17:29:25 | figleaf | johnthetubaguy: yes, that's been the plan | |
| 17:30:09 | johnthetubaguy | figleaf: but the code only reports the custom resource classes, or did I read that wrongly? | |
| 17:30:35 | cdent | johnthetubaguy, figleaf: can we restate the problem again (I know we keep doing that, but I think it is useful): If we released this code today without any change it would be wrong because: | |
| 17:31:31 | figleaf | cdent: ...once an ironic node reports resources with a custom resource class, all the VCPU etc. stuff gets removed | |
| 17:31:49 | figleaf | cdent: ...and we have no way to then select that ironic node | |
| 17:32:04 | figleaf | johnthetubaguy: cdent: Does that sound right? | |
| 17:32:06 | openstackgerrit | John Garbutt proposed openstack/nova: Block starting compute unless placement conf is provided https://review.openstack.org/425806 | |
| 17:32:12 | cdent | "reports resources" == writes inventory? | |
| 17:32:20 | figleaf | yeah | |
| 17:34:06 | figleaf | cdent: the _cleanup_ironic_legacy_allocations() method removes all the old-style inventory | |
| 17:34:48 | cdent | (if there was any) | |
| 17:35:04 | mriedem | the funny thing is, ironic + placement in newton doesn't work anyway | |
| 17:35:13 | figleaf | cdent: well, unless the node is newly-added, there will be | |
| 17:35:20 | mriedem | i.e. https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:stable/newton+topic:bug/1647316 | |
| 17:35:51 | cdent | figleaf: right, but when a node is newly added the same problem exists: there's no way to allocate or claim inventory? | |
| 17:35:51 | figleaf | mriedem: yeah - "funny" | |
| 17:36:10 | johnthetubaguy | mriedem: its almost like we need some ironic testing | |
| 17:36:27 | mriedem | johnthetubaguy: we have it in ocata | |
| 17:36:32 | mriedem | just not in newton where placement was optional | |
| 17:36:41 | johnthetubaguy | mriedem: ah | |
| 17:36:46 | mriedem | ironic + placement ci was broken for a few days in master | |
| 17:36:54 | figleaf | cdent: if a node is added and has a custom RC, its inventory will be added new-style | |
| 17:36:56 | mriedem | which is why i'm backporting that series of fixes | |
| 17:37:05 | cdent | figleaf: yes, but you can't allocate against it | |
| 17:37:06 | jroll | bah, no '-' in custom resource classes? :| | |
| 17:37:08 | figleaf | cdent: and we have no way to select that node now | |
| 17:37:16 | cdent | and that too | |
| 17:37:24 | cdent | feh | |
| 17:37:30 | mriedem | jroll: _ is far superior | |
| 17:37:49 | cdent | jroll: just upperletters and numbers | |
| 17:37:55 | jroll | and _ | |
| 17:38:19 | jroll | kinda lame IMO, but alas | |
| 17:38:26 | cdent | jroll: yeah, but that's just to keep you conused so best to just leave it out ... | |
| 17:38:55 | johnthetubaguy | figleaf: cdent: so the summary, either we have VCPUs, claim them, and schedule on them, OR we have to schedule on custom_resource classes and claim them, current state: schedule VCPU, report custom, claim VCPU (except for existing instances that we tidy up to claim the new resource class) | |
| 17:38:56 | cdent | I _think_ the constraint came into play when we didn't even have custom resource classes, not sure. | |
| 17:39:01 | openstackgerrit | Matt Riedemann proposed openstack/nova: Block starting compute unless placement conf is provided https://review.openstack.org/425806 | |
| 17:41:17 | jroll | well, unless I did something awful wrong, the ironic code gives me 2017-01-27 17:39:56.654 TRACE nova.compute.manager AttributeError: 'SchedulerClient' object has no attribute 'set_provider_inventory' | |
| 17:42:19 | jroll | nope, I'm at b6339a876a440831903445bd4b10afb732e91335 which is latest here | |
| 17:42:29 | johnthetubaguy | hmm, interesting | |
| 17:43:16 | cdent | this sounds like a job for functional tests | |
| 17:43:17 | figleaf | jroll: there are some places I found where SchedulerClient and SchedulerClient.reportclient are treated as the same | |
| 17:43:27 | figleaf | It's in reportclient | |
| 17:44:44 | jroll | figleaf: just reporting this code is broken :) | |
| 17:44:57 | jroll | cdent: I'm working on integration stuff atm | |
| 17:45:28 | figleaf | jroll: heh, can relate to that | |