| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-14 | |||
| 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 | |
| 21:52:11 | dansmith | mriedem: yes | |
| 21:52:28 | mriedem | mnaser: that or we unreserve the volumes for the bdms right there in conductor... | |
| 21:52:35 | mriedem | so that the api doesn't have to delete them locally | |
| 21:52:59 | mriedem | but then we get the "how many different nova services does it take to detach a volume" problem | |
| 21:53:01 | mnaser | that means that the volume will become available right away | |
| 21:53:06 | mnaser | that | |
| 21:53:06 | mnaser | but yeah | |
| 21:56:03 | mriedem | mnaser: melwitt: left notes on https://review.openstack.org/#/c/340614/ - thanks for the testing | |
| 22:01:21 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.61 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 22:01:56 | mnaser | mriedem: melwitt i can do both (though storing bdms seems easier).. i'll leave the decision up to you | |
| 22:11:48 | mriedem | before all of this we'd have the bdms created in the api along with the instance (before cellsv2 happened), so might as well just go with that for starters and see how it goes | |
| 22:12:11 | mriedem | not api, the 'nova' db, but the nova-api service would create them in the nova db | |
| 22:13:22 | melwitt | mriedem: sure, I can take the meeting baton tomorrow. let me know if I need to update the agenda or if you already did | |
| 22:13:32 | mriedem | i have not updated the agenda | |
| 22:13:46 | melwitt | k. I shall agenda then | |
| 22:15:22 | melwitt | and that sucks about the bdms thing. | |
| 22:15:39 | mriedem | right so we'll likely need to stagger these patches for sanity | |
| 22:15:54 | openstackgerrit | Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585 | |
| 22:15:56 | dansmith | efried: ^ | |
| 22:16:00 | mriedem | regression test, do something about bdms in conductor on scheduling failure, then the local delete api patch | |
| 22:16:03 | efried | dansmith: ack | |
| 22:16:04 | melwitt | yeah, I was about to say ... should mnaser take that issue as a separate patch? | |