Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-01-27
16:56:09 cdent johnthetubaguy: so if a new ironic thing comes into existence, with a defined, resource class, that's when we run into trouble?
16:56:18 figleaf johnthetubaguy: so they'll get handled like they were vms
16:56:21 johnthetubaguy cdent: yes
16:56:34 figleaf johnthetubaguy: ah
16:56:59 figleaf johnthetubaguy: so your concern is that we don't have the ironic-aware bits to select a resource provider?
16:57:06 mriedem andreykurilin: your upper-constraints dreams have come true https://review.openstack.org/#/c/426116/5/upper-constraints.txt@332
16:57:25 openstackgerrit Sarafraj Singh proposed openstack/nova: Adopts keystoneauth with glance client. https://review.openstack.org/412634
16:57:25 andreykurilin mriedem: \o/
16:57:46 johnthetubaguy figleaf: ish, its basically that this method will fail (looking for link)
16:58:45 johnthetubaguy figleaf: ah, so this will fail, when there is a new style ironic resource provider being used by a new instance that is created: https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L664
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 cdent which should mean that the ironic "instance" is not include in the loop that is calling update_instance_allocation
17:25:21 johnthetubaguy oh balls...
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 figleaf mriedem: yeah - "funny"
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: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.

Earlier   Later