| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-09 | |||
| 12:58:08 | bauzas | jaypipes: agreed, I was about to say the same, it's just a pattern discussion | |
| 12:58:14 | bauzas | nothing wrong from your side | |
| 12:58:18 | bauzas | just stylistic | |
| 12:58:34 | bauzas | that said, I did not -1d for that | |
| 12:58:51 | bauzas | I rather pointed the problem of explicitelly telling which columns to return | |
| 12:59:12 | bauzas | from a maintenability PoV, that is concerning me | |
| 13:00:33 | jaypipes | bauzas: and I responded that I prefer to have the columns be listed explicitly for the reason I outlined in my response to you. I gave you a specific example of what happens when you add a field to the ORM model and that field is a relationship() result and all of a sudden your queries that use that ORM model bloat up unnecessarily. | |
| 13:03:06 | bauzas | jaypipes: hum, seems like I missed that point then | |
| 13:03:13 | bauzas | if so, I apologize | |
| 13:03:26 | bauzas | jaypipes: in https://review.openstack.org/#/c/509025/2/nova/objects/resource_provider.py@405 ? | |
| 13:03:42 | jaypipes | yes | |
| 13:03:49 | bauzas | the Gerrit formatting messed up most of your comment | |
| 13:03:56 | jaypipes | bauzas: the comment I made on Oct 6 at 2:50pm | |
| 13:04:09 | jaypipes | bauzas: ugh, yeah :( | |
| 13:05:04 | bauzas | hah, I thought you were telling me *why* to not use ORM objects, which I don't disagree | |
| 13:05:38 | bauzas | I didn't noticed you were giving me an explanation about why it's important to be telling which fields to return | |
| 13:06:40 | jaypipes | bauzas: no worries. /me grabs breakfast and coffee now :) | |
| 13:07:36 | bauzas | jaypipes: /me needs to look again at your comment then | |
| 13:24:52 | openstackgerrit | Eric Fried proposed openstack/nova master: nova.utils.get_ksa_adapter() https://review.openstack.org/488137 | |
| 13:24:54 | efried | mriedem ^ | |
| 13:25:06 | efried | I worked on it all weekend | |
| 13:28:02 | mriedem | bauzas: i went through the bottom change in the "target host during cold migration" series again https://review.openstack.org/#/c/408955/ | |
| 13:28:03 | mriedem | fresh -1 | |
| 13:28:22 | efried | jaypipes Will you get a chance to skim https://review.openstack.org/510244 before the scheduler meeting? | |
| 13:28:51 | bauzas | mriedem: ack, needs to review as well | |
| 13:35:25 | dansmith | jaypipes: sdague: bauzas: This and the one right behind it are needed to unblock rally's gate, FYI: https://review.openstack.org/#/c/510203 | |
| 13:36:25 | bauzas | dansmith: looking | |
| 13:36:55 | sdague | dansmith: +2 | |
| 13:37:03 | sdague | dansmith: also, ug multiple sort queries | |
| 13:37:21 | dansmith | sdague: yeah, :/ | |
| 13:37:27 | sdague | that's mostly a good way to use no indexes | |
| 13:37:44 | dansmith | that | |
| 13:37:50 | dansmith | is another ibm special feature | |
| 13:37:57 | dansmith | but, the ship has sailed | |
| 13:39:22 | stephenfin | gibi: Single comment on https://review.openstack.org/#/c/483324/ | |
| 13:40:36 | gibi | stephenfin: thanks. | |
| 13:41:16 | gibi | stephenfin: I think that part of the code makes sure that the bdm is marked deleted in db | |
| 13:41:36 | stephenfin | gibi: But we still have the object reference that we can show? | |
| 13:41:38 | gibi | stephenfin: as we are at a last step of instance deletion | |
| 13:42:02 | gibi | stephenfin: yes we have the in memory version of that bdm | |
| 13:42:16 | gibi | stephenfin: or somebody could query it out form the db with read_deleted=True | |
| 13:42:58 | gibi | stephenfin: I can move the bdm.destroy() after the delete.end notification if that looks more logical to you | |
| 13:45:29 | mnaser | i'm trying to figure out from the commits if the placement database connection is something that is no longer being used? | |
| 13:46:05 | mnaser | it looks like it was reverted a while back.. but there's an open patch to add it again (that's pretty old): https://review.openstack.org/#/c/362766/ ? | |
| 13:49:23 | dansmith | mnaser: it is used by placement | |
| 13:49:27 | dansmith | er, wait | |
| 13:50:04 | mnaser | dansmith: https://review.openstack.org/#/q/39fb302fd9c8fc57d3e4bea1c60a02ad5067163f not sure if that helps you cobble things up together but it's not helping me much | |
| 13:50:06 | mnaser | (im reviewing the puppet change and seeing if we should deprecate that option or not) | |
| 13:50:29 | dansmith | mnaser: so we introduced some things a little too early and backed up a few steps, | |
| 13:50:46 | dansmith | but it's forward-looking not something that is going to go away | |
| 13:51:15 | dansmith | mnaser: we put something back in after that revert I think, just a sec | |
| 13:52:52 | dansmith | or maybe we never put it back | |
| 13:53:44 | dansmith | mnaser: well, anyway, "Deprecate" is probably the wrong word for the puppet patch, but it's confusing | |
| 13:54:25 | krypto | hello all any idea why windows vm created with 4 vcpu flavor shows only 2 vcpu inside it? I can see that in vm configuration its 4 cpu but inside the guest its just 2 | |
| 13:55:47 | mriedem | dansmith: looks like unit test failures in https://review.openstack.org/#/c/506416/ are real | |
| 13:55:59 | dansmith | mriedem: yep I know | |
| 13:56:25 | mnaser | dansmith: i think we'll add a warning to mention that it's currently un-used (not deprecated) till https://review.openstack.org/#/c/362766/ is merged | |
| 13:56:37 | mriedem | we never put the placement api stuff back in | |
| 13:56:39 | mriedem | after the revert | |
| 13:56:44 | mriedem | s/api/db/ | |
| 13:57:02 | edleafe | Scheduler subteam meeting in 3 minutes in #openstack-meeting-alt | |
| 13:57:24 | mnaser | mriedem so should we deprecate that option in puppet-nova or just warn that it's not used until it gets added again? | |
| 13:57:41 | dansmith | mriedem: I thought we had put an alternate connection url conf option in, that let you separate if not, but apparently we didn't | |
| 13:57:47 | mriedem | mnaser: i'd probably just say it's not used | |
| 13:58:12 | mriedem | otherwise we have to coordinate deprecations | |
| 13:58:24 | mriedem | and once puppet drops it, we might add it back in :) | |
| 13:58:37 | mnaser | alright cool, i'll leave a note mentioning to drop the warning once https://review.openstack.org/#/c/362766/ merges (if it ever does that is)? | |
| 13:58:58 | mriedem | we haven't put serious thought into what the migration / split out is going to look like | |
| 13:59:53 | mnaser | mriedem: gotcha. in that case we'll just leave it a warning and then keep up with nova's changes :> | |
| 14:00:05 | mnaser | thanks mriedem and dansmith :) | |
| 14:04:10 | stephenfin | gibi: Yeah, if you wouldn't mind | |
| 14:04:25 | gibi | stephenfin: sure, I can do that | |
| 14:04:27 | stephenfin | I get that it's safe there, but it seems...weird | |
| 14:04:30 | stephenfin | gibi: Ta :) | |
| 14:09:25 | gibi | mriedem: do you know if somebody is working on the spec discussed as a per requisite for the rx/tx queue size support? https://etherpad.openstack.org/p/nova-ptg-queens L239 | |
| 14:09:54 | gibi | mriedem: I reached out to nic but he said no | |
| 14:10:08 | mriedem | gibi: stephenfin: not sure that change needs to be made | |
| 14:10:12 | mriedem | at least not in the notifications patch | |
| 14:11:03 | mriedem | gibi: i thought cburgess and nic were working on that one | |
| 14:11:07 | gibi | mriedem: the notification patch started using the destroyed bdm and therefore introduced the confusion | |
| 14:11:15 | stephenfin | mriedem: It's not unrelated though - we weren't previously referencing bdm later in the function and now we are | |
| 14:11:16 | mriedem | i didn't follow the outcome of the conversation though | |
| 14:11:21 | stephenfin | Yeah, what gibi said | |
| 14:11:57 | gibi | mriedem: I pinged cburgess but no answer yet | |
| 14:12:02 | stephenfin | fwiw, I'm fine with it being done in a follow up, but I think it's confusing that we're "ensur(ing) block device mappings are not leaked", then "leaking" them via notifications | |
| 14:12:07 | gibi | cburgess: ^^ ping me please | |
| 14:12:32 | mriedem | stephenfin: well it means leaked as in not deleted from the db | |
| 14:12:50 | mriedem | thta's probably dead code now anyway | |
| 14:12:57 | mriedem | we delete the bdms when we delete the instance | |
| 14:13:25 | mriedem | https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L1891 | |
| 14:14:16 | mriedem | so given that, how about just removing that part in a follow up | |
| 14:14:21 | mriedem | rather than move it in this one | |
| 14:14:50 | stephenfin | Yup, let's do that so | |
| 14:15:16 | gibi | OK, I can push a followup | |
| 14:19:39 | mriedem | also just fyi https://github.com/openstack/nova/commit/5674e7646d106751b27d191e3334d9e6ebe9ab1b was newton, | |
| 14:19:47 | mriedem | so it should be safe to make that assumption from the queens compute | |
| 14:19:51 | mriedem | that the db api is doing what we want | |
| 14:20:12 | gibi | mriedem: ack | |
| 14:21:59 | mriedem | stephenfin: cool, thanks for hitting those | |
| 14:29:40 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix minor input items from previous patches https://review.openstack.org/506416 | |