| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-17 | |||
| 12:34:10 | alex_xu | for that case, actually we expect return that root RP twoice, and then we should generate two combinations: #1 root RP with local disk, #2 root RP with shared disk | |
| 12:34:35 | alex_xu | cdent: forgive me, let me use root rp first :) that pain for me also | |
| 12:34:57 | efried | alex_xu Actually, it won't return the compute+localdisk combination, because that wouldn't satisfy the RAID5 trait. | |
| 12:34:59 | alex_xu | and then, the trait implementation will find out #1 and #2 which have enough trait | |
| 12:35:37 | efried | But it *will* return the compute+shared combination, even though it *shouldn't*. | |
| 12:36:23 | efried | It shouldn't because the shared storage doesn't satisfy the SSD trait. But placement has no way of knowing that RAID & SSD were both supposed to apply to the storage, whereas the HW_CPU... trait wasn't. | |
| 12:37:56 | alex_xu | efried: I didn't have test it, but I think the current code will return compute+local | |
| 12:38:09 | alex_xu | and it never return the compute+shared combination | |
| 12:38:18 | efried | alex_xu Because of that bug? Maybe. | |
| 12:38:47 | alex_xu | because the implement of _get_all_with_shared | |
| 12:39:32 | alex_xu | but that still doesn't correct, you are expecting _get_all_with_shared return two combinations | |
| 12:39:56 | efried | _get_all_with_shared is the piece that doesn't yet take traits into account? | |
| 12:40:01 | alex_xu | and then the trait implement will filter out the correct one | |
| 12:40:07 | alex_xu | efried: yes | |
| 12:40:27 | efried | Right, gotcha. So you're right: _get_all_with_shared *should* return *both* combinations. (If it's not, that's a bug.) | |
| 12:40:51 | efried | And then the code you're writing *should* filter *both* of them out and yield zero candidates. | |
| 12:40:55 | alex_xu | efried: because this https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L895 | |
| 12:41:04 | efried | But there's no way to write your code to do that. | |
| 12:41:25 | alex_xu | that condition assumes that there only have the one case in (local and shared) | |
| 12:41:55 | alex_xu | efried: but the first thing we should figure out is that whether we want to support that case | |
| 12:41:57 | efried | ohh, you're missing a LEFT JOIN | |
| 12:42:21 | efried | Well, I contend we can't *not* support that case. | |
| 12:42:26 | efried | At least not long term. | |
| 12:42:56 | cdent | I’m going to save [t 3Vz8] because it is probably useful to whip out just about any time | |
| 12:42:56 | purplerbot | <efried> ohh, you're missing a LEFT JOIN [2017-10-17 12:41:57.325623] [n 3Vz8] | |
| 12:43:34 | efried | (c) 2017 | |
| 12:44:09 | efried | So alex_xu, if we were going to say we don't support that scenario, how would that be worded? | |
| 12:44:43 | alex_xu | at least, currently the nova doesn't support the case we have local disk and shared disk | |
| 12:45:06 | alex_xu | actually nova doesn't know about the disk is local or shared, it just create the instance image in a directory | |
| 12:45:37 | alex_xu | under that directory, it maybe local or a NFS... | |
| 12:45:52 | efried | alex_xu What about local ephemeral (boot) plus volumes? | |
| 12:46:10 | alex_xu | and I didn't have any real world case similar to that in my mind | |
| 12:46:34 | alex_xu | efried: the resource class DISK_GB is only about local ephemeral disk | |
| 12:46:54 | alex_xu | s/local ephemeral disk/ephemeral disk/ | |
| 12:47:54 | openstackgerrit | Eric Fried proposed openstack/nova master: placement: set/check if inventory change in tree https://review.openstack.org/470575 | |
| 12:48:45 | efried | alex_xu fixed nit ^ | |
| 12:49:33 | efried | cdent Did you want me to rename that 'p' var ^ ? | |
| 12:50:25 | efried | cdent I don't think we can afford to sweep this under the rug now and just hope it doesn't rear its head later. | |
| 12:50:32 | efried | Sorry, that was meant for alex_xu | |
| 12:50:56 | efried | (I *totally* think we can sweep a terse variable name under the rug) | |
| 12:51:11 | efried | ('p' for 'precedent'.) | |
| 12:51:26 | efried | So let's try this again. | |
| 12:51:36 | efried | alex_xu I don't think we can afford to sweep this under the rug now and just hope it doesn't rear its head later. | |
| 12:52:10 | efried | alex_xu At the moment, the scope of where this code is used is very contained, and possibly never even makes its way to where the user can see it. | |
| 12:52:44 | alex_xu | efried: sorry, I still didn't get you :), maybe...not use the 'rug', I didn't get what rug is reference to after search the dictionary.. | |
| 12:53:19 | efried | alex_xu But once we start allowing drivers to model their RPs and operators to specify their resources generically, this will wind up being a nontrivial bug we'll have to scramble to fix. | |
| 12:53:35 | cdent | efried: I’ve lost the fight on terse variable names, and surrender graciously to fight other battles another day. | |
| 12:53:40 | efried | alex_xu Sorry, idiom. "Sweep under the rug" means "ignore it for now and hope nobody notices" | |
| 12:53:59 | mriedem | johnthetubaguy: can i get you to take a look at this file injection deprecation spec? has a +2 from sdague, looking for another +2 https://review.openstack.org/#/c/509013/ | |
| 12:55:53 | alex_xu | efried: ok, you mean the shared RP case, right? we can file a bug for it | |
| 12:57:10 | openstackgerrit | Eric Fried proposed openstack/nova master: placement: set/check if inventory change in tree https://review.openstack.org/470575 | |
| 12:57:56 | alex_xu | I should read the 'sweep this under the rug', not just panic on the single 'rug'. | |
| 13:00:21 | efried | alex_xu I can't find an equivalent Chinese idiom, but perhaps 掩饰 gets close. | |
| 13:00:23 | openstackgerrit | Matt Riedemann proposed openstack/nova-specs master: Rename add-zvm-driver spec https://review.openstack.org/512606 | |
| 13:02:45 | openstackgerrit | Rodolfo Alonso Hernandez proposed openstack/os-vif master: Add memoize function using oslo.cache https://review.openstack.org/472773 | |
| 13:03:16 | alex_xu | efried: hehe, i get that | |
| 13:05:25 | efried | alex_xu Fixed that other docstring; cdent renamed the variable, just for you :) | |
| 13:06:40 | alex_xu | efried: cool, thanks | |
| 13:07:52 | alex_xu | efried: also leaved a question in https://review.openstack.org/#/c/510244/ | |
| 13:10:13 | alex_xu | ralonsoh: fyi first, I don't have +2 for the nova-spec :) | |
| 13:10:42 | ralonsoh | alex_xu: no problem! thanks for your time | |
| 13:12:19 | cdent | efried: the setUp in these compute manager related tests are … wow. Not the stuff you’ve added, but what was already there. | |
| 13:13:00 | efried | cdent I don't disagree. | |
| 13:13:08 | efried | I've got two more UTs to fix, btw. | |
| 13:13:12 | efried | But thanks for looking. | |
| 13:14:38 | cdent | efried: yeah, I know it’s not complete, I just went by because I wanted to understand the point of it all | |
| 13:15:15 | efried | cdent Hopefully the latest commit message makes it clear why we need this; let me know if not. | |
| 13:17:33 | cdent | yeah, it does | |
| 13:25:17 | efried | alex_xu Responded to that comment - let me know if you want to discuss. | |
| 13:25:35 | efried | alex_xu The code for looking stuff up in nested RPs - I'm not sure if that's part of Jay's series yet. | |
| 13:27:30 | alex_xu | efried: yea, I also want to ask the nested RP support for non-numbered request is implemented by which BP | |
| 13:27:52 | alex_xu | efried: I check the Jay's spec about nested RP, looks like that isn't included in the spec | |
| 13:28:17 | efried | alex_xu I remember talking about it - specifically about trait propagation - in some spec or other. | |
| 13:28:23 | efried | alex_xu Oh! It was *your* spec :) | |
| 13:28:39 | alex_xu | efried: for the question in your spec, is there any way we can try to find SRIOV_NET_VF and NET_EGRESS_BYTES_SEC from the same RPs for the non-numbered request? | |
| 13:28:53 | alex_xu | ok... | |
| 13:29:11 | alex_xu | sounds like we are missing one more spec? | |
| 13:29:48 | efried | alex_xu I don't see how that would work. Because then you would be ignoring the relationship between RPs in a tree. | |
| 13:29:51 | bauzas | sahid: there could be some regression in Pike for CPU pinning when you evacuate https://bugs.launchpad.net/nova/+bug/1723005 | |
| 13:29:52 | openstack | Launchpad bug 1723005 in OpenStack Compute (nova) "CPUPinningInvalid exception occurred when evacuate one instance repeatedly." [High,Confirmed] | |
| 13:30:15 | efried | alex_xu In other words, if you made it work that way, there would be all the "normal" cases that wouldn't yield any results. | |
| 13:30:35 | openstackgerrit | edan david proposed openstack/nova master: Don't overwrite binding-profile https://review.openstack.org/505613 | |
| 13:31:19 | alex_xu | efried: why | |
| 13:31:25 | openstackgerrit | Merged openstack/nova-specs master: Rename add-zvm-driver spec https://review.openstack.org/512606 | |
| 13:31:35 | sahid | bauzas: not sure we have supported that at some point | |
| 13:32:31 | efried | alex_xu Okay, can we agree that we would *not* want to get a candidate like { RP1: { SRIOV_NET_VF: 1 }, RP2: { NET_EGRESS_BW: 10000 } } ? | |
| 13:32:57 | bauzas | sahid: you mean evacuating an instance that was pinning a CPU ? | |
| 13:33:07 | bauzas | sahid: or any move ops ? | |
| 13:33:59 | alex_xu | efried: yes | |
| 13:34:34 | efried | alex_xu Then in order for that to work, you have to do your selection to make sure those inventories come from the *same* RP. | |
| 13:35:32 | efried | alex_xu So let's extend the model to include VCPU and MEMORY_MB. Those would be in the root RP. But the VF resources are still in the child RPs. | |
| 13:35:35 | alex_xu | efried: or to say try the best, if the RP doesn't have all resources, then looking for the child or shared | |
| 13:36:09 | sahid | bauzas: pinned instances... there were a patch serie pushed by nikola long time ago but never get merged unfortunatly | |
| 13:36:55 | bauzas | sahid: if you speak of live-migration claims, it's unrelated | |
| 13:36:57 | sahid | i think it never get merged because the work on-going with resource providers and placement :) | |
| 13:37:16 | efried | alex_xu Mm... that *could* work. | |
| 13:37:18 | bauzas | sahid: because we already claim on rebuilds | |
| 13:37:34 | openstackgerrit | Merged openstack/nova-specs master: Deprecate file injection https://review.openstack.org/509013 | |
| 13:38:12 | alex_xu | efried: and how the request with CPU and MEMORY_MB and SRIOV_NET_VF works? it try to the best? get CPU and MEMORY_MB from the root RP, and then try the child? | |
| 13:38:35 | sahid | bauzas: yes but that need to be checked whether we buil a new domain xml based on that new claim or not | |