Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-12
15:48:28 fried_rice But consuming update_provider_tree, I'm going to need to compare what's in the returned ProviderTree against what's in the report client's cached ProviderTree. So I need to peel e.g. the traits list out of the former so I have something to pass to have_traits_changed.
15:48:58 fried_rice And then if I decide it *has* changed, I'm going to need that same thing in order to send it down to placement.
15:49:25 bauwser superdan: I saw your comment on https://review.openstack.org/#/c/528832/7/nova/virt/libvirt/driver.py@4902, what would you prefer ?
15:49:40 bauwser superdan: I actually copy-pasted the Xen docstring
15:49:59 superdan bauwser: which comment?
15:50:02 superdan oh
15:50:05 bauwser L4902
15:50:06 gibi fried_rice: so you have a ProviderTree instance in the consumer and want to compare that with the ProviderTree instance in the cache
15:50:17 fried_rice giblet: just so.
15:50:34 superdan bauwser: oh that, I was just expressing frustration with the code, because I've been fighting with internal allocation stuff lately
15:50:42 superdan bauwser: not really asking for a change
15:51:05 superdan bauwser: it would be better if you put "allocations at microversion 1.x" I guess, but it's probably not worth it here
15:51:18 superdan bauwser: they changed format in 1.13 or something around there
15:51:22 gibi fried_rice: can we simply implement ProviderTree.diff(another_tree) function?
15:52:01 gibi fried_rice: do you need to know what is changed or you just have to updat what is changed?
15:52:14 gibi fried_rice: I mean update in the cache
15:52:19 fried_rice gibi a) that method would then still have to be able to get at another_tree.get_me_a_provider().get_me_its_fields(), and b) it would still have to return something that we can send to placement
15:52:22 bauwser superdan: okay, no worries :-)
15:52:30 bauwser I'll add more details and saying which rev
15:52:43 sean-k-mooney gibi: its an versioned object right so you can jsut call to primitive on both and then use the dict diff method to see if they are in sync or not
15:52:46 bauwser maciejjozefczyk_: my bad, just saw your ping
15:52:47 fried_rice gibi I have to update the cache too, but I also have to send changes back to placement.
15:53:01 bauwser gosh, I really need to resurrect my ZNC bouncer
15:53:20 bauwser the one I currently use is missing me notifications
15:54:03 gibi fried_rice: so you have to know which RPs are changed and also have to know what fields (e.g. inventory, aggregate, trait) are changed
15:54:07 bauwser maciejjozefczyk_: so we changed the opt values in order to signal whether it was changed by the operator or not
15:54:11 fried_rice gibi yup
15:54:38 fried_rice gibi None of this is a problem if I can get at the fields of the _Provider; but today there's no (legal) way to do that.
15:54:56 bauwser maciejjozefczyk_: now that there is no longer upgrade concerns with old Newton computes, I think it's okay to set that back
15:55:05 bauwser maciejjozefczyk_: thanks for helping on that !
15:55:22 sean-k-mooney fried_rice: no legal way because of the _
15:55:37 fried_rice sean-k-mooney Yeah, and the locking.
15:55:48 gibi fried_rice: OK, I think I understand the problem
15:56:40 fried_rice One idea I'm noodling with: Perhaps _Provider can return a read-only copy of itself.
15:57:13 fried_rice you know, def __setattr__(): raise
15:58:18 sean-k-mooney fried_rice: does the method that updates placement auto update the cache if so then ya get readonly copy and then update placement if out of sync and have it update teh cache
15:58:37 fried_rice sean-k-mooney Yeah
15:59:43 sean-k-mooney of course that is racy without a gloal lock on the provider tree e.g. the generation count
15:59:53 gibi fried_rice: read-only copy make sense. you can also add getters to ProviderTree returning read-only copy of the RP fields you need for your comparision
16:00:24 fried_rice gibi Yeah, that had been my initial thought; I just don't love the idea of having to have a zillion accessors
16:00:51 fried_rice If we do the copyout thing, we get new fields for free without having to implement new getters. That kind of thing.
16:03:21 gibi fried_rice: hm, if you copy the whoile internals of the ProviderTree then that might expose things that you don't want to expose (maybe new internal fields in the future)
16:04:08 fried_rice gibi _Provider, not ProviderTree. And I would be okay with handling that by making the internals of _Provider (or ReadOnlyProvider, or whatever) private.
16:04:50 figleaf fried_rice: perhaps make a read-only class that implements the copy from the _Provider?
16:05:07 figleaf just copy the relevant fields
16:05:25 fried_rice figleaf Yuh. Though I'm having trouble figuring out how to make a read-only class that you can still initialize :)
16:05:46 gibi fried_rice: in general, if you do bulk copy you get every new thing by default and that can be too much. If you do selective copy the you will get no new things automatically which might be not enough
16:05:57 finucannot melwitt: Could you take another look through https://review.openstack.org/#/q/topic:bp/websocket-proxy-to-host-security today? Think I've answered all your questions on the base patch
16:06:18 gibi fried_rice: so both copy strategy has its own edge case
16:06:48 sean-k-mooney fried_rice: if you make a dedicated class it dows not need to be read only. just have a method that create a opject of ReadOnlyProvider form the provider
16:06:53 figleaf fried_rice: make the __setattr__() conditional. Start with writing enabled, initialize, and then flip the switch. Once flipped, you can't flip it back
16:07:12 fried_rice figleaf Yeah, that should work. Playing...
16:07:29 melwitt finucannot: yes, thanks for the replies
16:07:52 fried_rice sean-k-mooney Yeah, so like when you get it, you could technically modify it, but it wouldn't affect the original.
16:09:15 mriedem finucannot: melwitt: so am i ok to start reviewing the websocket-proxy-to-host-security series?
16:09:41 finucannot mriedem: Can't speak for melwitt, but I think so, yes
16:09:44 mriedem ok
16:10:17 mriedem running https://review.openstack.org/#/c/530950/ again
16:10:46 sean-k-mooney fried_rice: yep you could enven just use the same Provider class if you wanted just make a deep copy of it and return the copy
16:11:01 melwitt finucannot: did you link the wrong thing in your reply here about what fixed the original py35 job failure we saw? https://review.openstack.org/#/c/531834
16:11:01 fried_rice sean-k-mooney Just so.
16:11:16 sean-k-mooney fried_rice: that would be wasting some ram but either way you cant modify the original
16:11:35 melwitt finucannot: because that link is to the same review. oh, you're saying you combined them into one review
16:11:48 melwitt I got it
16:11:49 finucannot mriedem: Go for it. I wonder if we can enable that by default in the gate at some point (i.e. override the DevStack default)
16:11:55 finucannot melwitt: yup, 'zactly
16:12:18 fried_rice sean-k-mooney I have to copy anyway. I don't want to lock down the original. I still want to be able to modify it through legal means (via ProviderTree)
16:12:18 finucannot In hindsight, I probably should have kept it with the change that actually exercises the code (like it was), but it's done now
16:12:21 mriedem finucannot: we should enable it in the nova-next job
16:12:25 mriedem like in melwitt's patch
16:12:32 finucannot Can move back if you'd rather that
16:12:35 mriedem https://review.openstack.org/#/c/513160/
16:12:48 melwitt finucannot: okay so it looks like you only have test coverage for one of the convert-to-bytes and not the other?
16:13:06 fried_rice figleaf That conditional setattr thing works a treat, thanks for that.
16:13:10 mriedem melwitt: the test coverage for both would be in the devstack change testing all of this on the top right?
16:13:17 fried_rice (whether I end up using it or not)
16:13:20 finucannot melwitt: In that change, yes, because there aren't yet any callers for 'sendall'
16:13:36 finucannot The callers come in latter patches and the tests are the functional ones
16:13:46 finucannot ...which caught the issue in the first place
16:13:58 mriedem honestly i'm mostly interested in seeing clean ci runs and getting the good enough version of this in
16:14:27 finucannot mriedem: I thought nova-next was a neutron thing, i.e. run neutron with the next version of nova to make sure they still talk nicely?
16:14:35 mriedem no
16:14:49 mriedem nova-next == nova with fancy wizbangs
16:14:52 figleaf fried_rice: kewl
16:15:03 melwitt mriedem: yeah. we were talking about unit tests for the bytes send stuff
16:17:12 melwitt finucannot: I see. I would think the unit test coverage should have gone along with the addition of 'sendall' originally, it's just we didn't anticipate anything being wrong with the context passed to sendall
16:18:27 melwitt but yeah. I agree to focus on the devstack testing at this point
16:20:00 melwitt s/content/context/
16:20:30 gibi mriedem: second try looks good now in https://review.openstack.org/#/c/533210/2 , I will create the proper patches (one for nova and one for the project-config) on Monday
16:20:39 melwitt guh s/context/content/
16:21:21 finucannot melwitt: Yeah, there's no tests for 'TenantSock' because it was supposed to be just a shim around 'websockify.websocket.WebSocketRequestHandler'. Perhaps it's time we added them.
16:22:07 melwitt finucannot: yeah, I realized the same. we should do that as a follow up later
16:24:19 superdan bauwser: if you didn't see the pings from yesterday, this needs your eyes (and -2?) https://review.openstack.org/#/c/532924/
16:26:18 melwitt mriedem: on my consoles series, my reply was basically that we currently only invalidate console auths for delete instance and live migrate. and I was thinking not to change any behavior in my series since it's supposed to just be changing the backend of the console auth
16:27:09 melwitt I think you're right that we should be invalidating console auths for all instance moves including cold migrate and shelve but I was thinking that would be a separate change
16:28:23 cdent mriedem: nova-next as py3 makes excellent and vaguely poetic sense
16:28:42 mriedem cdent: did you just see my comment on stephen's patch?
16:28:45 cdent yes
16:28:48 mriedem heh
16:28:55 openstackgerrit Balazs Gibizer proposed openstack/nova master: Make sure that functional test triggered on sample changes https://review.openstack.org/533210

Earlier   Later