Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-12
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 fried_rice sean-k-mooney Just so.
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: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 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: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: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
16:28:56 openstackgerrit Balazs Gibizer proposed openstack/nova master: DNM: Testing if funct test is triggered https://review.openstack.org/533211
16:29:12 mriedem cdent: i think we can make that happen once newton is dead
16:29:27 mriedem as nova-next should only run on pike+
16:37:57 openstackgerrit Eric Berglund proposed openstack/nova master: Add PowerVM to feature-classification https://review.openstack.org/533237
16:38:37 mriedem finucannot: got some nits in https://review.openstack.org/#/c/345397/ but those could be dealt with in a cleanup patch at the end of the series
16:39:17 finucannot mriedem: Cool. I can tackle those now
16:42:50 openstackgerrit Andreas Jaeger proposed openstack/nova master: Make sure that functional test triggered on sample changes https://review.openstack.org/533210
16:43:10 fried_rice gibi That host1 != host2 thing - is the fix merged at this point?
16:43:20 openstackgerrit Andreas Jaeger proposed openstack/nova master: DNM: Testing if funct test is triggered https://review.openstack.org/533211
16:44:02 superdan several things in the gate have failing tests and zuul doesn't seem to be responding to status queries
16:44:13 superdan might be headed for another reset
16:44:29 superdan oh yep, here it comes
16:45:16 fried_rice gibi Found it: https://review.openstack.org/#/c/533190/ -- Guess I can rebase on top of it, or wait for it to merge.
16:54:19 mriedem finucannot: cool. some more in https://review.openstack.org/#/c/345398/
16:54:32 finucannot Sweet
16:57:35 mriedem finucannot: oh boy, what is this about https://review.openstack.org/#/c/345399/42/nova/cmd/novncproxy.py@43 ?
16:57:50 mriedem is that asserting that is cool for all other virt drivers besides xen/
16:57:51 mriedem ?
16:58:56 finucannot Yup, melwitt and I discussed that. The drivers that support VNC support 3.8, and the ones that don't support VNC don't matter, obviously. It's only Xen that supports 3.3
16:59:19 mriedem ok
16:59:50 finucannot Yup, here's the thread https://review.openstack.org/#/c/345399/34/nova/cmd/novncproxy.py@43
17:07:07 openstackgerrit Eric Fried proposed openstack/nova master: WIP: ProviderTree.find => ReadOnlyProvider https://review.openstack.org/533244
17:07:22 fried_rice cdent figleaf gibi sean-k-mooney There's a start ^
17:13:46 mriedem finucannot: some questions in https://review.openstack.org/#/c/345399/
17:15:07 mriedem melwitt: i've gone through the meaty parts of the websocket proxy series
17:15:20 mriedem withholding +2 on the last rfb one until the questions are answered and CI results are done
17:15:59 cdent fried_rolls: noted, got it on my list for a looksee
17:18:06 melwitt mriedem: just skimmed through your comments. originally, finucannot had enabled TLS for only the libvirt driver and I suggested changing it to not xen because I thought generally when we add these features we don't aim to target them at one driver only and that some consensus around at least more than one driver is reached before going ahead
17:18:33 melwitt maybe I was mistaken on that
17:19:58 openstackgerrit Mathieu Rohon proposed openstack/nova master: Allow VMs to use unaddressed port https://review.openstack.org/533249
17:31:16 finucannot mriedem: I've addressed all the questions. Will tackle the cleanups in the follow-up patch
17:33:05 sean-k-mooney fried_rolls: the patch seams fine - the lack of any testing :)
17:35:40 sean-k-mooney fried_rolls: instead of _check_readonly and the current __setattr__ you could just asign a throwing __setattr__ at the end of the init. it might save some cycle during the copy of the childern that said the branch predictor should get this right 99% of the time
18:32:55 openstackgerrit Mathieu Rohon proposed openstack/nova master: Allow VMs to use unaddressed ports https://review.openstack.org/533249
18:41:52 mriedem melwitt: no you're correct that we want features adopted by a majority of virt drivers
19:04:12 mriedem melwitt: ok i'm +2 up through the rfb patch
19:04:22 mriedem and ci is happy http://logs.openstack.org/50/530950/1/check/legacy-tempest-dsvm-py35/6f0c24d/logs/screen-n-novnc.txt.gz#_Jan_12_17_42_32_200099
19:06:24 melwitt mriedem: sweet, I'll go through it again today
19:09:26 mriedem finucannot: comments in the docs patch and i'm done https://review.openstack.org/#/c/500544/
19:10:34 finucannot mriedem: Excellent, thanks :) I'll address all the comments in a follow-up by Monday
19:10:54 mriedem ack
19:11:01 mriedem tell danpb the good news :)
19:11:07 mriedem he is risen

Earlier   Later