Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-13
09:37:12 lyarwood mdbooth: morning, https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:master+topic:bp/cinder-new-attach-apis should be ready for reviews once you have time today, the focus is really on the db and object changes first if you wouldn't mind focusing on them.
09:38:16 openstackgerrit Thomas Bechtold proposed openstack/nova master: Log exception when hypervisor version check fails https://review.openstack.org/434927
09:45:47 dakhmetov Greetings, guys!
09:45:57 dakhmetov Could you please review this patch: https://review.openstack.org/#/c/399679/
09:50:47 dakhmetov I know a couple of people who are really affected by the inconsistency in instance's libvirt.xml. It will be great if you help me reviewing it.
09:54:06 johnthetubaguy dakhmetov: so I love half of your change, and worry about the other half
09:54:34 johnthetubaguy dakhmetov: using instance.user_id and instance.project_id totally makes sense, we should have done that for the beginning
09:55:06 johnthetubaguy username and projectname... thats feels wrong, what about we don't add that any more?
09:55:07 dakhmetov 2 johnthetubaguy please share your concern
09:55:16 johnthetubaguy I will add a note on the patch
09:55:23 gmann Qiming: admin pass can be injected while creating server and change password using action API is for xen and libvirt as alex_xu mentioned
09:55:48 johnthetubaguy dakhmetov: a related comment came up on friday at a HPC meetup I went to, so agreed it needs fixing
09:55:55 openstackgerrit Michael Henkel proposed openstack/nova master: Implements: blueprint opencontrail-dpdk-support https://review.openstack.org/441183
09:56:39 gmann @Qiming not sure about exact SDK requirement on that and if any issue on nova side?
09:58:48 johnthetubaguy dakhmetov: I am worried about how usernames and project names change over time in keystone, but we keep those names just matching what they were went the instance booted
09:58:58 johnthetubaguy dakhmetov: maybe thats OK for your use case I guess?
10:00:22 dakhmetov 2 johnthetubaguy: yes, I also thought about it. The only option to avoid this issue is open Keystone session there each time
10:01:48 johnthetubaguy dakhmetov: my idea was to only have the ids in there, from the instance object, and leave the others blank for now?
10:02:31 johnthetubaguy that would ensure they are "correct" in some sense, but I feel bad breaking the folks that want the names there
10:04:59 dakhmetov johnthetubaguy: sure, there are a lot of folks who needs names there, as it described in nova xmlns 1.0
10:07:53 dakhmetov so, what we have in current patch seems to be best solution, despite the fact that the stored user and project names can change
10:08:01 mdbooth dakhmetov: I've reviewed a change related to this recently. Do you have a link?
10:08:49 dakhmetov mdbooth: exactly! https://review.openstack.org/#/c/399679
10:09:09 mdbooth Ah, ok. You fishing for a second +2?
10:09:33 dakhmetov mdbooth: yes, a kind of :)
10:11:12 mdbooth johnthetubaguy: My take on this is that we break things by rebuilding the libvirt xml. We shouldn't be doing that, and this is a necessary workaround.
10:11:45 mdbooth i.e. the conversation isn't so much about what these values mean, whether they're correct, or what they really should be
10:11:52 mdbooth But more: are they the same as before
10:12:22 mdbooth The other conversation is also interesting, though.
10:12:29 johnthetubaguy mdbooth: my worry is the bug that says I updated the project or user name in keystone, and now my lib virt xml is out of sync, for me its either we don't include the names or we do what dakhmetov has proposed
10:13:16 johnthetubaguy totally agreed grabbing it from the context was exactly the wrong thing to do
10:14:17 johnthetubaguy I was meaning the bug that is raised next time, after we fix this bit
10:14:29 mdbooth Ah, ok.
10:14:38 mdbooth So, that bug was always present
10:15:02 johnthetubaguy yep
10:15:04 mdbooth This information is static, so storing dynamic information there is suspect at best.
10:15:18 johnthetubaguy yeah, thats my take
10:15:33 mdbooth And I agree, and probably that we should remove username and projectname
10:15:47 mdbooth However, that's tangential to this bug, which is that we break our existing contract
10:15:57 johnthetubaguy cool, I was wondering if I was going down a crazy thought path
10:17:11 mdbooth dakhmetov: Incidentally, if you're close to users who are actually relying on this, I'd be interested to know what your take is on how to fix the changing username issue.
10:17:38 johnthetubaguy +1 thats what I was fishing for really
10:18:40 mdbooth dakhmetov: Is the issue that people have scripted this, and scripts will require updating?
10:19:10 johnthetubaguy FWIW, this change breaks those folks too
10:19:28 mdbooth johnthetubaguy: Really? I missed that.
10:19:43 johnthetubaguy all existing instance, once modified will now have no username or projectname, we are missing a unit test for that
10:20:42 mdbooth johnthetubaguy: Ah, yes. It's a fix only for future instances, I'd glossed over that.
10:21:07 mdbooth We could save the info before deleting it...
10:21:28 mdbooth Meh, lets get this done before gold plating it.
10:25:14 johnthetubaguy dakhmetov: I updated my comments to try summarise what I am thinking now, basically I would split this into two patches, and the second patch needs an extra unit test.
10:25:16 mdbooth dakhmetov: Can you quickly write the test case johnthetubaguy has asked for? Should be simple enough and probably just works, but we should definitely be testing that.
10:32:39 alex_xu Qiming: gmann, yea, i mean it even worse, they have similar name, but doing different thing
10:40:19 dakhmetov mdbooth: johnthetubaguy: reading your comments
10:41:35 dakhmetov as workaround for outdated names in instance's system_metadata: names can be updated in mysql
10:42:28 dakhmetov it's dirty hack, but anyway solves the possible issue
10:44:03 mdbooth dakhmetov: I think the safer option is to test that having nothing there at least doesn't break existing instances. That means that we're not regressing. Saving the info before rewriting the xml is better, but also a game of whack-a-mole, so best be safe first I think.
10:49:48 kashyap lyarwood: Isn't melwitt right here -- https://review.openstack.org/#/c/441204/5/nova/tests/unit/virt/libvirt/test_guest.py
10:50:01 dakhmetov mdbooth: So, what I need is to handle the situation with missing owner_user_name and owner_project_name in instance's metadata, as well as add test for this. Right?
10:50:05 kashyap Specifically her question about 'live=False'
10:50:36 johnthetubaguy dakhmetov: as long as we test for that, thats my main worry out the way
10:50:47 mdbooth dakhmetov: I suspect it already handles that, but we should definitely test as johnthetubaguy says.
10:50:56 mdbooth If it doesn't work, that would be a regression.
10:52:12 johnthetubaguy dakhmetov: yeah, I think the get() is OK, but deep down, I would rather we didn't populate the names at all myself, hence me being tempted to do this as two patches, one that just gets user_id and project_id from the instance, sets the others to None, then a second patch that adds in your cache of the names.
10:54:05 mdbooth johnthetubaguy: How about resolving username and projectname from keystone at the time we rewrite the metadata?
10:54:15 mdbooth Rather than storing it in system meta
10:54:18 openstackgerrit Rodolfo Alonso Hernandez proposed openstack/nova master: Read NIC features in libvirt https://review.openstack.org/444415
10:54:24 mdbooth Would that be so bad?
10:56:02 openstackgerrit Stephen Finucane proposed openstack/nova-specs master: Reserve NUMA nodes with PCI devices attached https://review.openstack.org/364468
10:57:16 dakhmetov mdbooth: if we open Keystone session to resolve ids to names in libvirtxmdriver with token from request context, it may not have permissions to resolve names in keystone
10:57:46 dakhmetov we may not have permissions to resolve IDs to names in keystone even if we always open admin session
10:57:52 dakhmetov it's how I see it
10:57:57 mdbooth dakhmetov: Explain
11:00:19 mdbooth dakhmetov: I'm not being funny, I just don't understand that interaction all that well.
11:01:44 dakhmetov mdbooth: keystone policies are adjustable in a wide range
11:01:47 dakhmetov https://github.com/openstack/keystone/blob/master/etc/policy.json
11:02:17 mdbooth dakhmetov: Where did the lookups come from originally?
11:02:55 mdbooth Are you saying that the policy context is different between creation and subsequent lookup, or that somebody might have changed the policy?
11:03:04 dakhmetov mdbooth: so, listing users or projects can even be prohibited
11:04:18 mdbooth dakhmetov: It would be a pretty odd policy which denied an authenticated user permission to lookup their own username though, right?
11:05:07 mdbooth "identity:get_project": "rule:admin_required or project_id:%(target.project.id)s",
11:05:17 mdbooth "identity:get_user": "rule:admin_or_owner",
11:05:43 johnthetubaguy dakhmetov: I don't think we should call out to keystone in there
11:06:26 mdbooth johnthetubaguy: I know that came up earlier in the review, but are you thinking of performance or robustness when you say that?
11:06:33 johnthetubaguy dakhmetov: I would rather we get the patch in that uses instance.project_id and instance.user_id first, thats an important fix (we could use the context)
11:07:02 johnthetubaguy mdbooth: a combination of all of those really, plus security around every compute node having those permissions, and setting that up correctly
11:07:56 johnthetubaguy mdbooth: matching uuids to current names feels like something the monitoring systems could do
11:08:07 openstackgerrit jichenjc proposed openstack/nova master: Trivial Fix https://review.openstack.org/442310
11:09:49 openstackgerrit jichenjc proposed openstack/nova master: Add description for Image location in snapshot https://review.openstack.org/440379
11:16:32 openstackgerrit jichenjc proposed openstack/nova master: check hw:mem_page_size when change extra_spec https://review.openstack.org/408610
11:18:17 bauzas johnthetubaguy: oh, just saw https://review.openstack.org/#/c/442387/1/resolutions/20170307-compute-platform.rst
11:18:42 bauzas johnthetubaguy: I haven't seen any ML discussion around this
11:18:57 bauzas johnthetubaguy: is this something you discussed in ATL ?
11:19:09 johnthetubaguy bauzas: I haven't started any, its something I have been wanting to suggest for a while
11:19:32 bauzas johnthetubaguy: oh okay
11:19:44 bauzas fine by me, just wanted to make sure people were happy with it
11:20:03 johnthetubaguy bauzas: I suspect people hate it, but I think its needed
11:20:45 cdent johnthetubaguy: it's gonna rub some people funny, but I agree it is important
11:21:51 johnthetubaguy I think we keep avoiding difficult conversations for two long on this kind of thing
11:22:04 bauzas ++
11:22:13 bauzas anyway, /me goes offline

Earlier   Later