| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-12 | |||
| 19:23:02 | sean-k-mooney | efried: im just heading home but i addem myself to the review and ill leave it open in a tab for tomorow. ill give it a quick skim before i leave however | |
| 19:24:18 | dansmith | efried: looks okay to me | |
| 19:25:46 | sean-k-mooney | efried: why did you start with "Traits are special. Rather than overwriting the entire set of traits" this statement also applies to RPs and Aggregates | |
| 19:26:17 | efried | sean-k-mooney: Because I wrote this before it occurred to me that we should be doing the same for the other bits. | |
| 19:26:21 | sean-k-mooney | efried: the compute node RP is not owned by nova so nova cant override all child RPs of the compute node either | |
| 19:26:29 | sean-k-mooney | efried: ah ok | |
| 19:26:51 | efried | sean-k-mooney: But I also wanted to do this one isolated because I don't think we're done discussing the others. | |
| 19:27:07 | efried | The compute node RP *is* owned by nova. | |
| 19:27:24 | efried | sean-k-mooney: And *some* of its children may also be. | |
| 19:27:51 | dansmith | yeah, the compute node RP is _definitely_ owned by nova :) | |
| 19:28:05 | efried | In general virt needs to be smart enough not to muck with stuff it doesn't own. But originally we *thought* that meant virt owns everything about the RPs it owns. | |
| 19:28:07 | sean-k-mooney | efried: that has issues wich other services want to tag it with traits | |
| 19:28:08 | dansmith | agreed nova can't blow away all the providers underneath, unless it's deleting the compute node | |
| 19:28:28 | sean-k-mooney | dansmith: well event then i dont think it can | |
| 19:28:36 | efried | Which is why we already have ProviderTree methods to add/remove providers in the tree. | |
| 19:28:42 | efried | ...individually. | |
| 19:28:49 | dansmith | sean-k-mooney: it has to be able to delete it and the subtree, IMHO | |
| 19:29:44 | sean-k-mooney | dansmith: if i have a converged deployment where my compute nodes are also cinder storage pool providers the the root node of the tree would be a parent of the nova resources and the cinder resouces | |
| 19:29:52 | efried | Agree. E.g. if neutron created bandwidth RPs under a PF, and compute removes the PF, the bandwidth providers are no longer relevant. | |
| 19:30:05 | sean-k-mooney | which is why i dont think the root node can be owned by nova | |
| 19:30:14 | efried | sean-k-mooney: I don't think that's a model we should support. | |
| 19:30:16 | dansmith | sean-k-mooney: in that case the cinder providers are not servicing other computes, right? then it's okay to delete it | |
| 19:30:28 | dansmith | sean-k-mooney: if they are servicing others, then the cinder pool should not be a child of the compute node | |
| 19:30:45 | sean-k-mooney | dansmith: no the could be and associated with other computes via a sharing aggregate | |
| 19:30:58 | dansmith | sean-k-mooney: if they are, they should not be a child of the compute IMHO | |
| 19:31:20 | jaypipes | efried: I agree with both edleafe and dansmith on that update to the u-p-t spec | |
| 19:31:28 | dansmith | sean-k-mooney: the compute node RP is not the physical computer, it's the nova service and the hypervisor/virt driver underneath | |
| 19:32:00 | sean-k-mooney | dansmith: perhaps i had always just envisioned that the server itself was a resouce provider all services could create childeren under without having to worry about it beeing deleted by e.g. nova | |
| 19:32:24 | dansmith | sean-k-mooney: not, IMHO.. if we need to model that relationship then the compute node RP would be a child to "the computer" I think | |
| 19:32:29 | jaypipes | dansmith: well, in the case of a hypervisor host, yes. :) for ironic, of course, that's different. | |
| 19:32:33 | dansmith | but I hope we don't need to do that | |
| 19:32:48 | sean-k-mooney | dansmith: yes well that has some advantages | |
| 19:32:58 | dansmith | jaypipes: yeah I know, but thats special | |
| 19:33:06 | jaypipes | ack | |
| 19:34:06 | dansmith | jaypipes: ironic nodes wouldn't be cinder providers in the same hierarchy, and even better, the computer running the nova service for those, if it were a cinder pool shared with other nodes, should be modeled peer to the compute service's RP I think | |
| 19:34:26 | sean-k-mooney | dansmith: the only issue i have really with saying that nova owns the root RP is that we may need to have multiple root nodes for the same server depending on the service | |
| 19:34:48 | dansmith | sean-k-mooney: no, nova owns the compute RP.. if you decide to make that the root of your cinder provider, then it is a root :) | |
| 19:34:59 | dansmith | and you get what's coming to you in that case :) | |
| 19:35:06 | dansmith | which is nova may delete the root that it owns | |
| 19:35:30 | sean-k-mooney | dansmith: yes but the we have too trees for the same phyical server and we dont currently have a way to model that they are the same server | |
| 19:35:37 | sean-k-mooney | i guess maybe with an aggregate | |
| 19:35:47 | dansmith | sean-k-mooney: they're not the same server, | |
| 19:36:02 | sean-k-mooney | dansmith: why not? | |
| 19:36:10 | dansmith | sean-k-mooney: they represent services that happen to be on the same box.. if we need to model that they're on the same box (i.e. below a parent provider) then that's a thing | |
| 19:36:10 | efried | dansmith, jaypipes: Please confirm: you want two separate methods, with *traits args | |
| 19:36:17 | dansmith | efried: that's fine | |
| 19:36:18 | jaypipes | efried: yes please | |
| 19:36:26 | efried | ight | |
| 19:36:34 | jaypipes | add_traits() and remove_traits() would be my preference. | |
| 19:36:43 | jaypipes | efried: ^ | |
| 19:36:45 | dansmith | efried: btw, excellent job not having your head explode here. kudos :) | |
| 19:37:19 | efried | jaypipes: Cool, swhere I was gonna go. dansmith: ack, thx; head may yet explode over aggregates thing, but so far so good. | |
| 19:37:21 | dansmith | efried: on friday of PTG, I thought maybe your eyeballs were about to shoot from your head at lethal velocity, which scared me when they were pointed at me | |
| 19:37:43 | sean-k-mooney | dansmith: ok anyway thats a little off topic. i do think we may want to have a parent of the compute node RP at some point then but let cross that bridge only when we need too | |
| 19:38:11 | dansmith | sean-k-mooney: we may and that would solve the problem of this ownership of the tree for deletes problem, that's what I'm saying :) | |
| 19:38:32 | dansmith | until that point, if you create a provider underneath someone else's provider, I think you need to know that you're at the other's mercy | |
| 19:38:35 | efried | dansmith: It ain't my eyeballs that should worry you :P FWIW, I'm still uncomfortable with this, but I don't see a better alternative. | |
| 19:38:46 | dansmith | um. | |
| 19:39:46 | dansmith | oof | |
| 19:40:48 | sean-k-mooney | dansmith: oh yes it would solve that issue. it would raise the question of which tree i shoudl create the resouce other for thinks like bandwith. anyway i better run before the stores close at 8 | |
| 19:41:14 | dansmith | yeah | |
| 19:52:19 | mnaser | hm, does anyone have any idea behind the historical reason why ComputeCapabilitiesFilter is hard-coded to use instancetypes (not taking image properties into consideration?) | |
| 19:52:50 | mnaser | context: customer *needs* instances with a specific cpu flag, i'd like to minimize the number of flavors so i was hoping i can have a image flag requesting that cpu flag | |
| 19:53:00 | mnaser | vs having to create a new instance type for this use case specifically | |
| 19:53:48 | efried | jaypipes: Considering removing ProviderTree.set_traits/set_aggregates entirely. | |
| 20:00:18 | openstackgerrit | Eric Fried proposed openstack/nova-specs master: Allow for merging traits and aggregates https://review.openstack.org/552122 | |
| 20:00:32 | efried | dansmith, jaypipes, cdent, edleafe: ^ with changes as requested, plus for aggregates | |
| 20:03:16 | cdent | ✔ | |
| 20:16:00 | jaypipes | efried: +2 from me. thx | |
| 20:16:09 | efried | thx | |
| 20:21:54 | edleafe | efried: a little late, but added my +1 for good measure :) | |
| 20:22:03 | efried | thx | |
| 20:23:02 | dansmith | efried: consider me suitably thrown under the bus for my method naming | |
| 20:24:57 | efried | dansmith: You're not held accountable for sample code in pastebin. Else I would have griped at you about having the + case first and the - case the default. | |
| 20:25:56 | dansmith | else should have raised | |
| 20:26:37 | mriedem | efried: with ^ how can i integrate that with https://review.openstack.org/#/c/538498/ where i'm not in a virt driver with a provider tree - or will that RT code eventually have the provider tree object so i can call add_traits there? | |
| 20:28:05 | mriedem | alternatively, if the RT isn't the place to do that and the virt driver is, i could add a generic method in the base ComputeDriver class that handles adding traits from the capabilities dict | |
| 20:28:29 | dansmith | mriedem: virt driver can do that from u_p_t he's describing here | |
| 20:28:45 | mriedem | right, but i don't want to copy the same thing in all virt drivers | |
| 20:28:49 | dansmith | but better for compute to do it right where it's calling into the virt driver | |
| 20:28:51 | mriedem | so RT or base class, i'm ok with either | |
| 20:28:52 | dansmith | call the capabilities compute-owned | |
| 20:29:03 | efried | mriedem: I'm glad you asked. I believe you should get the traits from placement and add in those you got from _get_traits (which is the union of those from virt & capabilities). | |
| 20:29:06 | mriedem | the compute isn't calling this, the RT is | |
| 20:29:10 | dansmith | yeah, wherever we call that in the virt driver from | |
| 20:29:22 | mriedem | i don't know what the RT will have the UPT | |
| 20:29:25 | dansmith | efried: eh? | |
| 20:29:29 | mriedem | maybe it will and i'm just behind | |
| 20:29:49 | mriedem | efried: i'm not sure why i need to call placement, | |
| 20:29:53 | dansmith | efried: we get them from the virt driver structure, we can just generate the traits that go with them and tack them onto the provider tree for the compute node RP | |
| 20:29:56 | mriedem | i want to *always* report these capabilities | |
| 20:29:58 | dansmith | mriedem: you wouldn't | |
| 20:29:59 | dansmith | right | |
| 20:30:09 | dansmith | these are compute-asserted traits, IMHO | |
| 20:30:56 | mriedem | telling me, "go look at the UPT series of changes and figure it out" is acceptable also | |
| 20:31:42 | efried | mriedem: If we can slot your change into the UPT series, that would make things easier to reason about. Having to decide what it would look like in all three possible permutations is harder. | |
| 20:32:04 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Network interface attach/detach https://review.openstack.org/546813 | |
| 20:32:10 | dansmith | mriedem: https://review.openstack.org/#/c/520246/52/nova/compute/resource_tracker.py L884 there I think | |
| 20:32:43 | dansmith | logically it's compute, but mechanically it's RT because that's what owns the compute node RP | |
| 20:32:52 | mriedem | ok that works for me, i can rebase on top of that | |