| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-23 | |||
| 12:48:52 | openstackgerrit | jichenjc proposed openstack/nova master: Remove duplicate error info https://review.openstack.org/510719 | |
| 12:56:52 | cdent | maybe we should put traits in etcc and make them watchable | |
| 12:56:59 | cdent | jk/2 | |
| 12:57:15 | cdent | s/etcc/etcd/ | |
| 13:01:05 | openstackgerrit | Stephen Finucane proposed openstack/nova master: zuul: Move legacy jobs to project https://review.openstack.org/514309 | |
| 13:12:13 | bauzas | efried: traits are not boolean | |
| 13:12:41 | efried | bauzas In the sense that they're True if they're present and False if they're absent, kinda? | |
| 13:12:45 | bauzas | efried: turning off a capability means we remove the trait | |
| 13:13:02 | efried | oh, no, I'm pretty sure that's the entire bone of contention here. | |
| 13:13:13 | bauzas | lemme respond on the ML thread | |
| 13:13:36 | efried | Though to be precise, you're correct: if we remove a *capability* (which is different from turning off a setting), we should remove the trait from that RP. | |
| 13:13:52 | efried | bauzas Which is consistent with what I said, though I should have been explicit about it. | |
| 13:13:56 | cdent | removing and turning off not the same thing | |
| 13:14:08 | efried | Right. | |
| 13:14:24 | efried | If I make it so the RP can no longer do the thing, that should come with removing the trait. | |
| 13:14:38 | efried | If the RP is still capable of doing the thing, and I just switch it off, I should leave the trait in place. | |
| 13:15:02 | efried | But even capable vs. on/off is a sticky thing with ironic. | |
| 13:15:36 | efried | Should more precisely say "capable of being turned on during spawn". | |
| 13:16:00 | bauzas | cdent: a trait just means "we support that capability" | |
| 13:16:14 | bauzas | if we don't support it, then we remove the trait | |
| 13:16:24 | cdent | yes, that’s what I was saying | |
| 13:16:26 | bauzas | we couldn't support it because we had turned off something | |
| 13:16:42 | bauzas | but then, if we turn off something, then we delete the trait | |
| 13:16:44 | bauzas | that's MHO | |
| 13:17:02 | bauzas | exactly like a CPU feature | |
| 13:17:13 | efried | bauzas Agree, as long as "we" is e.g. "the admin", not something that can be turned on via the spawn process. | |
| 13:17:15 | cdent | hmm, I suspect it depends on what is doing the action, and whether changing it can be automated | |
| 13:17:22 | efried | cdent ++ | |
| 13:17:24 | cdent | jinx-ish | |
| 13:18:27 | efried | I think we're all in agreement here. Just needs to be spelled out carefully. | |
| 13:21:43 | openstackgerrit | Jan Zerebecki proposed openstack/nova master: Only log not correcting allocation once per period https://review.openstack.org/508262 | |
| 13:24:30 | alex_xu | efried: hi, i'm here | |
| 13:25:12 | efried | alex_xu I wanted to coordinate with your refactoring of AllocationCandidates.get_by_filters. | |
| 13:25:34 | alex_xu | efried: yea | |
| 13:25:41 | efried | alex_xu As I started implementing the numbered grouping syntax... | |
| 13:25:57 | alex_xu | efried: ok...hope you didn't start too much... | |
| 13:26:38 | efried | alex_xu https://review.openstack.org/514091 and https://review.openstack.org/514092 | |
| 13:27:04 | efried | alex_xu First off, I'm completely changing the format of the 'filters' parameter. That's described in that first patch. | |
| 13:27:58 | alex_xu | efried: ok, that's cool | |
| 13:28:03 | efried | alex_xu Which I was going to implement, at least at first, by creating a separate method in AllocationCandidates - get_by_filters_grouped | |
| 13:28:34 | efried | But of course, get_by_filters_grouped would be able to use a lot of the same functions you're refactoring out of get_by_filters, which is good. | |
| 13:29:19 | efried | alex_xu The place we need to coordinate initially, I think, is the parsing of the querystring to create the filters - that first patch. | |
| 13:29:50 | alex_xu | efried: ok, I will read your patch | |
| 13:29:53 | efried | alex_xu Because at some point you were going to make changes to parse the 'required' query param to feed into your series. | |
| 13:30:00 | efried | alex_xu You haven't started that work yet, have you? | |
| 13:30:27 | alex_xu | efried: I have, https://review.openstack.org/#/c/479776/ but didn't update recently | |
| 13:31:07 | efried | alex_xu Okay, I'll read that one over. | |
| 13:31:22 | alex_xu | efried: we probably need to put the schema and parse code of 'required' as common | |
| 13:31:46 | alex_xu | efried: do you have any sample code for your get_by_filters_grouped | |
| 13:31:57 | alex_xu | I guess that is the hard part | |
| 13:32:21 | efried | alex_xu Right, my thought was that we might as well do it all at once. That is, make the API change from accepting just 'resources' to accepting 'resources[N]' + 'required[N]' in one microversion jump. | |
| 13:33:35 | alex_xu | efried: since microversion is micro, we shouldn't put too much in single version. but that should be ok | |
| 13:33:36 | efried | alex_xu No, I haven't written any of that yet. The easy part will be grabbing the candidates for a single numbered grouping. Because (per the spec) that's always coming from a single RP - I don't need to mess with aggregates or trees. | |
| 13:34:22 | efried | alex_xu Slightly harder will be combining that with the results from the unnumbered group. But even that shouldn't be *too* hard - we just need to match up the aggregates at that point. | |
| 13:35:22 | efried | alex_xu But I haven't done any serious SQL in ~15y, and never done any sqlalchemy, so I'm definitely going to be looking for guidance/help/mentoring there. | |
| 13:36:06 | alex_xu | efried: do you read the patch I just submitted https://review.openstack.org/#/q/topic:bp/add-trait-support-in-allocation-candidates+(status:open+OR+status:merged)? | |
| 13:36:34 | alex_xu | I created few objects to instead of currenlty single huge method | |
| 13:37:03 | efried | alex_xu I saw those come through, but haven't looked them over yet. But that's what prompted me to ping you. | |
| 13:37:28 | alex_xu | the AggregatedProviderSummary obj will be the one to indicate a set of providers which are in the same aggregate | |
| 13:37:55 | alex_xu | so I'm think whether we need other object to build a tree for nested resource provider | |
| 13:38:08 | efried | alex_xu That already exists: ProviderTree | |
| 13:38:22 | alex_xu | efried: ProviderTree is for nova, not for placement | |
| 13:38:31 | efried | oh, right. | |
| 13:38:58 | cdent_ | why do we need to represent the tree on the placement side? (not saying we don’t just want to clarify on why) | |
| 13:39:21 | efried | cdent_ Not strictly necessary, but sure does make it easier to conceptualize. | |
| 13:39:31 | efried | cdent_ For things like trait propagation | |
| 13:39:31 | cdent_ | “it”? | |
| 13:39:46 | cdent_ | I’m still not entirely clear on the reality of trait propagation | |
| 13:40:01 | cdent_ | especially in a world of a numbered resources | |
| 13:40:53 | mriedem | dansmith: i think the bottom alternate hosts change is ready https://review.openstack.org/#/c/486215/ | |
| 13:40:57 | mriedem | you and jay were +2 before | |
| 13:40:59 | alex_xu | emm...any hint about trait propagation? my brain becomes empty, not sure due to the time or not... | |
| 13:41:00 | efried | cdent_ There was an example in the spec. Possibly in review comments rather than in the actual text. And it may not have been a great example, at that. | |
| 13:41:34 | efried | alex_xu cdent_ It may have been something like this: | |
| 13:42:38 | efried | alex_xu cdent_ I have multi-port SR-IOV NICs associated with NUMA nodes. Each port is connected to a different physnet. | |
| 13:42:59 | dansmith | mriedem: ack, I wanted to be happy with the usage of the object before we merged the one that added it, but I guess that bottom one is separate | |
| 13:43:18 | efried | alex_xu cdent_ So my model looks like: compute => numa node => PF | |
| 13:44:46 | efried | alex_xu cdent_ The NUMA node has a trait indicating which node it is, and it holds the VCPU and MEMORY_MB inventory. The PF has a trait indicating which physnet it's on, and it holds the VF inventory. | |
| 13:45:33 | efried | alex_xu cdent_ But the logic still needs to be able to know that the PF belongs to the NUMA node associated with its parent RP. | |
| 13:45:50 | alex_xu | efried: the NUMA node needn't a ttrait to indicate, but nvm, let us continue your example | |
| 13:46:10 | alex_xu | efried: yea, probably | |
| 13:46:16 | efried | Perhaps NUMA is a bad example because you wouldn't be making a request for a specific NUMA node. This is more about affinity, which is for later. | |
| 13:46:39 | efried | Let me go see if I can dig up the example from the spec... | |
| 13:47:18 | alex_xu | efried: cdent_ I'm thinking we may need a obj just like this for shared rp https://review.openstack.org/#/c/514200/1/nova/objects/resource_provider.py@2552 to show the path for nested resource provider | |
| 13:47:32 | alex_xu | so I mean we needn't build a tree | |
| 13:47:53 | alex_xu | what we need a build a list of path for the condicates | |
| 13:48:08 | alex_xu | s/condicates/candidates | |
| 13:48:31 | alex_xu | what we need is that building a list of pathes for the candidates | |
| 13:48:44 | efried | You mean every possible "branch"? | |
| 13:48:53 | alex_xu | efried: yea, branch | |
| 13:49:29 | efried | In the general case that could be a big list, but in reality I suppose it would probably be manageable. | |
| 13:50:04 | alex_xu | shrink the scope of all the branches by the sql, then filter the final branch in the code | |
| 13:50:49 | alex_xu | if we want to done everything in the sql, that will be super complex sql just like current we have, and only Jay know how to write a such sql :) | |
| 13:50:57 | efried | alex_xu cdent_ Here's the example from the spec discussion: https://review.openstack.org/#/c/497713/6/specs/queens/approved/add-trait-support-in-allocation-candidates.rst@42 | |
| 13:53:37 | alex_xu | efried: I see | |
| 13:53:57 | alex_xu | not really propagation, but probably that is rule how we check each branch | |
| 13:54:04 | cdent_ | efried: yeah, I remember that. I think my issue isn’t so much with behavior as with terminology. | |
| 13:54:30 | efried | cdent_ We already spent a ton of emotional energy haggling over "propagate" vs "inherit". | |
| 13:54:36 | cdent_ | I agree that in the request example, you should get back a tree that includes the NUMA, and the compute node. that’s not quite the same as "hit" | |
| 13:55:20 | efried | cdent_ Well, in that context, "hit" means we actually got that candidate. As opposed to NOT getting that candidate because the NUMA RP doesn't have the CPU trait itself. | |