| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-03 | |||
| 12:59:38 | sean-k-mooney | and as an unprivaldaged user i cant tell if its cross cell or not | |
| 13:00:10 | Luzi | thank you, that was reasonable the best review I got so far :D | |
| 13:01:10 | sean-k-mooney | :) well hopefully we can get this landed soon. as i said the rest of the spec looks good to me | |
| 13:02:13 | sean-k-mooney | just to undersdand the problem with option 2, what is the issue with createing keys automatically | |
| 13:02:52 | sean-k-mooney | i had assumed we would use the same key we are currently using | |
| 13:03:23 | sean-k-mooney | rather then create a new one but i also have not considered it much | |
| 13:03:37 | Luzi | who will delete a newly created key bound to an image? | |
| 13:04:17 | Luzi | well if its not exactly the same image anymore, would you encrypt it with the same key? | |
| 13:04:47 | sean-k-mooney | well for shelve we delete the image we created when unshelving | |
| 13:05:20 | Luzi | so there would be a point for nova to create and delete a key, that would be okay | |
| 13:05:25 | sean-k-mooney | and for cross cell resize we auto delete teh image snapthot when we create the instnace on the dest node | |
| 13:05:26 | sean-k-mooney | so nova | |
| 13:06:01 | sean-k-mooney | yes in both cases nova could create a temperoy key and it could delete it at the correct time | |
| 13:06:02 | Luzi | i know that discussion from cinder and glance, there cinder creates a key which glance has to delete - that is not a good practise | |
| 13:06:51 | sean-k-mooney | right that is messy | |
| 13:07:36 | Luzi | sean-k-mooney, that seems good from my perspective, so automatic key creation for shelve and cross-cellresize, given a metadata parameter indicating a formerly encrypted image | |
| 13:08:06 | sean-k-mooney | yep and if that metadata paramter is not set just do what we do today | |
| 13:08:17 | Luzi | yes | |
| 13:08:25 | sean-k-mooney | so you can make it complete transparent to the end user and no api changes needed | |
| 13:09:37 | Luzi | that sounds good, I will add this to the spec :) | |
| 13:16:16 | lyarwood | mdbooth: https://review.opendev.org/#/c/668750/ - thoughts on this? Came across it last night while looking at upstream bugs and suddenly noticed this codepath is dead. We could just keep it but it hasn't been tested in ~3 years so it might be easier to start over with a clean volume driver. | |
| 13:18:18 | mdbooth | lyarwood: Hmm. It doesn't sound like we're going to implement native iscsi any time soon, tbh, due to lacking multipath and no upstream motivation to implement it. | |
| 13:18:35 | mdbooth | Making this just dead code which isn't like to be used any time soon. | |
| 13:18:42 | mdbooth | And which probably doesn't work anyway. | |
| 13:19:51 | mdbooth | lyarwood: Is os-brick based iscsi problematic? | |
| 13:20:42 | lyarwood | mdbooth: it has some rough edges wrt to multipath etc but the basic single path stuff also covered by this native support is QEMU is pretty solid | |
| 13:20:53 | lyarwood | s/is/in/g | |
| 13:21:23 | lyarwood | so yeah I'm fine nuking this for now and if anyone cares enough we can reintroduce it later in a broken out volume driver | |
| 13:21:30 | lyarwood | and have it actually tested in the gate etc | |
| 13:21:41 | mdbooth | lyarwood: +1 from me for nuking unusable code | |
| 13:22:21 | lyarwood | ack thanks | |
| 13:25:54 | shilpasd | efried: Hi, need discussion on https://review.opendev.org/#/c/667952/1/nova/conf/scheduler.py@188 | |
| 13:26:07 | efried | Sure shilpasd | |
| 13:26:29 | efried | I'm sure we must have discussed this during the spec review | |
| 13:26:30 | shilpasd | here saying 'enable_forbidden_aggregates_filter' conf option bydefault enabled | |
| 13:26:46 | efried | It must be about the performance penalty. | |
| 13:26:54 | shilpasd | efried: but what we observed for other request filters, its not so | |
| 13:27:33 | efried | Well, this one is doing a pretty expensive db lookup every time. The other filters are (I think) exclusively local processing. | |
| 13:27:48 | efried | Have you done any kind of performance analysis of this filter? | |
| 13:27:54 | efried | especially on a large database with lots of aggregates | |
| 13:28:39 | shilpasd | efried: not on that large scale | |
| 13:29:13 | efried | dansmith: you were also concerned about the performance of this filter. | |
| 13:30:14 | efried | Having thought about it some, we're better off being conservative unless we can demonstrate that the db lookup is super cheap. | |
| 13:30:25 | efried | So - leave the conf option as is shilpasd :) | |
| 13:30:30 | efried | I'll update the review. | |
| 13:31:07 | sean-k-mooney | efried: i think all the prefilters default to false so for consitency i would keep it as false too | |
| 13:31:21 | shilpasd | efried: great, thank you, we are keeping it as False | |
| 13:31:24 | sean-k-mooney | at least until we have info from deploymetns that its cheap and works well | |
| 13:31:56 | efried | sean-k-mooney: In this case I was suggesting that we remove the toggle entirely. You have to do enough other stuff to opt in, the only benefit you get by having it switched off is that you don't run the code. | |
| 13:32:05 | shilpasd | sean-k-mooney: thank you for opinion | |
| 13:32:31 | sean-k-mooney | ah right ok | |
| 13:33:17 | shilpasd | efried: thanks for discussion | |
| 13:33:22 | efried | yw | |
| 13:44:01 | gibi | stephenfin, efried: I'm +2 on the extra_spec validation spec https://review.opendev.org/#/c/638734 . alex_xu and bauzas had comments earlier. do we want to wait for them to chime in? | |
| 13:44:31 | stephenfin | gibi: alex_xu, sure, but I think bauzas is MIA now since it's summer and he's French | |
| 13:44:58 | gibi | stephenfin: OK, cool with me | |
| 13:46:04 | efried | probably want to get sean-k-mooney to re-ack as well | |
| 13:46:24 | sean-k-mooney | which reivew? | |
| 13:46:30 | efried | he has the medal for most comments in a single review on that one. | |
| 13:46:33 | efried | sean-k-mooney: https://review.opendev.org/#/c/638734 | |
| 13:46:37 | efried | extra spec validation | |
| 13:46:54 | sean-k-mooney | ah ok | |
| 13:47:10 | sean-k-mooney | am ya that is a medal i get a little too often | |
| 13:47:48 | sean-k-mooney | ill quickly re review it now | |
| 13:49:32 | efried | stephenfin: any merit in changing the topic of that spec and its patch (https://review.opendev.org/640733) to ...-extended to match the new bp? | |
| 13:49:48 | efried | I could understand if you want to keep them grouped with the old ones, as currently. | |
| 13:49:51 | efried | either way | |
| 13:49:57 | stephenfin | I'm happy with either too | |
| 13:50:14 | stephenfin | so I'll leave it but won't object if you want to change it :) | |
| 13:50:21 | efried | I only glanced at the number of results https://review.opendev.org/#/q/topic:bp/flavor-extra-spec-image-property-validation+(status:open+OR+status:merged) -- no idea how related/unrelated they actually are. | |
| 13:50:38 | efried | is your work really a continuation of that? | |
| 13:50:55 | stephenfin | To be honest, not really now. It's the same idea but on a broader scale | |
| 13:51:14 | stephenfin | Perhaps I should have a totally different name. Open to suggestions | |
| 13:51:39 | stephenfin | Given that I'm not doing image metadata validation now, a new name would definitely be a good idea | |
| 13:51:40 | efried | Oh, let's not go through the paperwork of changing the blueprint and stuff. | |
| 13:51:54 | efried | but yeah, that's a point | |
| 13:55:02 | sean-k-mooney | cool your going with my stevador suggestion for cyborge extra spec valdaditon or other services. | |
| 13:55:05 | sean-k-mooney | almsot done | |
| 13:55:19 | stephenfin | yeah, it was a good one | |
| 13:57:12 | efried | sean-k-mooney: I saw (but did not read) you discussing image encryption with Luzi. Where are you on that spec? | |
| 13:57:34 | efried | I noticed she added a sentence on (no) impact on lifecycle operations; is that sufficient to address your concern from the previous PS? | |
| 13:58:14 | efried | also lyarwood: are you +1 on that? Your words imply so, but there's no vote. | |
| 13:58:37 | efried | talking about https://review.opendev.org/#/c/608696/ btw | |
| 13:58:47 | sean-k-mooney | efried: nova should create a tempory key on shelve and encypte the snapshot if the instance image was encypted and delete the key when unshelving. same for cross cell resize | |
| 13:58:49 | Luzi | efried, i have to reconsider shelve and cross-cell resize | |
| 13:59:00 | lyarwood | efried: yeah sorry the comments were against an older PS so I couldn't vote | |
| 13:59:15 | efried | okay, cool, thanks all. I'll hold off and wait for a new PS | |
| 13:59:27 | sean-k-mooney | either that or we need to expsoe it on the shelve/resize api endpoint for the end user to decide | |
| 14:00:39 | openstackgerrit | Dakshina Ilangovan proposed openstack/nova-specs master: Spec: Provider config YAML file https://review.opendev.org/612497 | |
| 14:00:53 | efried | sean-k-mooney, Luzi: But that changes everything. | |
| 14:01:28 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/queens: Init HostState.failed_builds https://review.opendev.org/668911 | |
| 14:01:30 | sean-k-mooney | efried: what changes everything? auto encypte or api endpoint or both | |
| 14:01:49 | efried | Auto encrypt: As previously written, you didn't keep track of whether a server was originated from an encrypted image. Now you would have to. | |
| 14:01:49 | efried | API endpoint: now you need a microversion (right?) | |
| 14:02:06 | sean-k-mooney | yes | |
| 14:02:26 | sean-k-mooney | too ^ we would need a microversion | |
| 14:02:44 | sean-k-mooney | not sure about keeping track of if it was encypted | |
| 14:03:01 | sean-k-mooney | i think Luzi said there was a metadta key on the image/instance | |
| 14:03:22 | efried | I thought the spec said we forget about that. | |
| 14:03:27 | sean-k-mooney | i assume it would be in the embeded copy of hte image meta we store in the request spec | |