| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-14 | |||
| 21:32:40 | dansmith | *ah | |
| 21:32:58 | dansmith | efried: you could have flavors you allow BFV from but that aren't dedicated to bfv | |
| 21:32:59 | efried | dansmith: Getting the trait from not-flavor to avoid Skittles. | |
| 21:33:47 | dansmith | efried: so if you want to make sure that BFV requests using generic flavors don't get routed to any old host, you might want to slap a trait on those requests so that you can send them to the right place | |
| 21:33:59 | mriedem | my idea was that the pre-placement filter thing dan's spec is adding, would shove a trait into the flavor embedded in the reqspec b/c that's currently how we get the traits from the user to the scheduler to placement | |
| 21:34:28 | efried | Right, so we're merging traits-from-flavor with traits-from-elsewhere | |
| 21:34:32 | dansmith | we could do that too | |
| 21:34:41 | mriedem | so if we know we're doing bfv on a multiattach volume (or a tagged bdm), this code puts a CUSTOM_COMPUTE_SUPPORTS_MULTIATTACH on the flavor within the reqspec | |
| 21:34:47 | mriedem | efried: yeah | |
| 21:35:10 | efried | mriedem: Not on the flavor - on the allocation_candidates request | |
| 21:35:12 | mriedem | we could always do something else like the pre-placement filters add RequestSpec.system_traits (like system_metadata) or something | |
| 21:35:25 | mriedem | efried: but we have to get them to the allocation candidates request somehow | |
| 21:35:26 | dansmith | efried: on the flavor in the reqspec if we want it to be honored for a resize or migration kinda thing | |
| 21:36:25 | efried | Do we make other "modifications" to flavor data between when we pull it from the flavor db and when we attach it to the instance? | |
| 21:36:36 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove old flavor_create db api method https://review.openstack.org/544703 | |
| 21:36:51 | dansmith | efried: we suggested doing this for zero-root-disk BFV things I think | |
| 21:37:12 | dansmith | efried: we also probably need traits to be stored in the reqspec proper for when the user asked for them right/ | |
| 21:37:22 | mriedem | https://review.openstack.org/#/c/428505/ ? | |
| 21:37:54 | dansmith | mriedem: no, mine below that | |
| 21:37:56 | efried | My point is that it feels kinda weird for me to say 'boot an instance with this flavor, that has traits X,Y,Z' and then when I look at the flavor on the instance later, it also has traits A,B,C. I'm like where tf did those come from? | |
| 21:38:09 | mriedem | efried: you wouldn't see those | |
| 21:38:15 | efried | Okay, then I'm cool. | |
| 21:38:25 | efried | If they're on the RequestSpec and not on the flavor per se. | |
| 21:38:26 | mriedem | the instance.flavor would show the public facing flavor you used to create the instace | |
| 21:38:45 | dansmith | instance.flavor not reqspec.flavor | |
| 21:38:46 | mriedem | right, they'd be on the flavor embedded within the request spec, which doesn't get exposed out of the api today | |
| 21:38:55 | mriedem | ? | |
| 21:39:11 | mriedem | GET /servers/{id}/detail shows the flavor embedded in the instance | |
| 21:39:17 | dansmith | although we probably create instance.flavor from reqspec.flavor | |
| 21:39:28 | mriedem | we don't | |
| 21:40:05 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L910 | |
| 21:40:12 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L891 | |
| 21:40:19 | efried | So yeah. dansmith What I'm saying is that this is an alternative that should be at least documented in the Alternatives section; *and* some more explanation along these lines is needed for the reader to make sense of the statement on L47 that mentions traits for the first/last/only time. | |
| 21:40:20 | dansmith | it doesn't really matter anyway, as that'd be a conversation for how we're going to do a thing with traits, | |
| 21:40:32 | dansmith | unrelated to the general idea of augmenting the request to placement | |
| 21:40:57 | dansmith | efried: I'm not sure I get what is an alternative | |
| 21:41:10 | dansmith | efried: traits for the tenant isolation is not an option, IMHO | |
| 21:41:33 | dansmith | if you mean it's an alternative to the "and also maybe later we could do a bfv thingy", then agree, but it's not really the subject of this spec | |
| 21:41:46 | efried | So | |
| 21:41:53 | mriedem | example," | |
| 21:41:53 | mriedem | would enable other such request-limiting filters in the future. For | |
| 21:41:53 | mriedem | "Although not covered by this spec directly, the mechanism added here | |
| 21:42:00 | mriedem | that seems pretty clear that this is a framework type thing | |
| 21:42:08 | mriedem | the spec is dealing with the specific tenant aggregate problem, | |
| 21:42:10 | dansmith | this could have been two specs, but I felt it was way better to not have a tiny spec for each | |
| 21:42:14 | mriedem | but is generic enough that we can build on it for other problems | |
| 21:42:32 | mriedem | other problems like bfv | |
| 21:42:33 | dansmith | maybe I'm missing what efried is saying is an alternative though | |
| 21:43:43 | mriedem | i think he's saying, if i have A, B, C in aggregate X, rather than model it as an aggregate, the resource providers for A,B,C would have an X trait | |
| 21:43:49 | mriedem | which is a PITA | |
| 21:43:53 | mriedem | so i agree we don't want to do that | |
| 21:44:10 | dansmith | for tenant isolation for sure | |
| 21:44:11 | efried | therefore at least closer to being a viable option, and should therefore be described in the Alternatives section, and dismissed with an explanation as to why it's being dismissed if indeed there's a good reason for that. | |
| 21:44:11 | efried | dansmith: I agree that [traits for tenant isolation if you have to specify the traits in flavors] is not an option - but you describe it in Alternatives and explain why it's not an option (Skittles). IMO, [traits for tenant isolation where we merge the traits from the RequestSpec that we got from conf just like you're talking about doing with aggregates] *may* also not be viable, but it doesn't suffer from your main objec | |
| 21:44:53 | mnaser | mriedem: good call on suggesting functional test. while the codebase seems to actually not stop there, it looks like it's not getting any bdms in _delete() .. BlockDeviceMappingList(objects=[]) | |
| 21:45:03 | efried | And then you get the added bonus of being able to refer to that model from L46-7, because otherwise L46-7 doesn't make sense in the context of the rest of the spec. | |
| 21:45:06 | mnaser | is it possible that bdms are created later? :< | |
| 21:45:18 | dansmith | efried: ah okay, that's what I'm saying is also not an option IMHO, but for other reasons than flavor explosion | |
| 21:45:21 | mriedem | mnaser: i bet i know why, | |
| 21:45:27 | mriedem | mnaser: i bet the bdms aren't buried in cell0 | |
| 21:45:32 | efried | dansmith: Cool, please say that in the spec and I'm good. | |
| 21:45:50 | mnaser | yay.. | |
| 21:45:58 | mriedem | mnaser: just a guess, but that's my guess | |
| 21:45:58 | mnaser | should they be buried in cell0? | |
| 21:46:01 | dansmith | efried: because we have to label each compute individually with the trait and I think that's what aggregates are for, but yep, I'll put it in as a slightly different thing.. just for you :) | |
| 21:46:20 | efried | Thanks dansmith. Kisses. | |
| 21:46:40 | mriedem | it's a valentines day miracle | |
| 21:46:54 | efried | dansmith: Having to label each compute individually with the trait - what I'm confused about on L46-7 is why that argument doesn't apply for BFV too. | |
| 21:47:47 | dansmith | efried: well, the way I phrased that is silly, since that's traits all up and down, but let me say it in the spec and then you can argue about it, if it's clear | |
| 21:47:51 | mriedem | efried: for one thing, nova-compute can report the BFV traits capabilities automatically, the operator / admin doesn't have to do that for every tenant | |
| 21:48:00 | mnaser | select * from block_device_mapping => Empty set | |
| 21:48:09 | mnaser | i guess i'll get them added.. | |
| 21:48:19 | efried | mriedem: It can do that for host aggs too, though, nah? | |
| 21:48:34 | dansmith | mriedem: and compute can report aggregates automaticaly | |
| 21:48:47 | mriedem | that's an up-call for one thing | |
| 21:49:19 | mriedem | nova-compute would have to get the aggregates that the host is in, find the tenant_id meta on those, and convert that to traits on the RP | |
| 21:49:22 | efried | mriedem, dansmith: Oh, so yeah, that was my point: we're talking about automatically mirroring host aggs as placement aggs; in the alternative I describe above, we would be instead automatically mirroring host aggs as traits on the compute node RP. | |
| 21:49:22 | mriedem | i agree we could do that, | |
| 21:49:24 | mriedem | but it's an up-call | |
| 21:49:53 | mriedem | the bfv trait is much more clear to me as a 'trait', | |
| 21:50:03 | dansmith | efried: right, but then you create a tenant trait, apply to all computes. if you want to reassign a cell to a different tenant, you remove/reapply against all those computes | |
| 21:50:04 | mriedem | the tenant isolation thing is very much less a 'trait' in my head | |
| 21:50:07 | dansmith | efried: compared with: | |
| 21:50:25 | efried | ...same but for agg associations | |
| 21:50:27 | dansmith | efried: this cell is aggregate cell-foo, and add "tenant bar is allowed" to the cell-foo aggregate, | |
| 21:50:44 | mriedem | mnaser: oh i know the problem, | |
| 21:50:46 | dansmith | efried: then you can reassign a cell to a tenant in one place instead of mass add/remove of the other trait | |
| 21:51:04 | efried | Sorry, I don't get the difference. | |
| 21:51:10 | dansmith | efried: hangout? | |
| 21:51:11 | efried | If the trait is CUSTOM_CELL_FOO | |
| 21:51:12 | mriedem | mnaser: so in conductor schedule_and_build_instances, we call the scheduler for a host and if we get one, we look up what cell that host is in and create the instance and bdms in that cell, | |
| 21:51:24 | efried | dansmith: Sure | |
| 21:51:25 | mriedem | mnaser: if scheduling fails, we create the instance in cell0, | |
| 21:51:30 | mriedem | but we don't create the bdms in cell0 | |
| 21:51:32 | mnaser | but not the bdms | |
| 21:51:49 | mriedem | exactamundo | |
| 21:51:50 | mriedem | melwitt: ^ | |
| 21:51:52 | dansmith | efried: required https://hangouts.google.com/call/vebCoylDfu_0tvlCDPwmAAEE, edleafe mriedem jaypipes optional | |
| 21:52:04 | mriedem | pants required? | |
| 21:52:04 | mnaser | i guess i'll look into making it create bdms in cell0 | |