Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-05-30
19:50:29 mriedem i've been addressing my own nits to keep it going
19:50:31 mriedem so yeah he won't mind
19:50:34 mriedem no
19:50:41 artom "rebaes"
19:50:52 mriedem if you rebase like you spell, we might have problems
19:50:53 artom When you come back to your bae
19:51:03 tssurya mriedem, dansmith: thanks for the reviews, I just got back from a (2 weeks long) vacation and also don't have anything for the meeting. Will catch up on the summit stuff from tomorrow.
19:51:16 dansmith tssurya: cool
19:51:41 mriedem tssurya: rest assured we talked you up big time to belmiro and tim in vancouver
19:51:56 tssurya mriedem: haha thanks a ton!!
19:52:42 mriedem artom: just: git review -d 458820 && git rebase -i HEAD~2
19:52:52 mriedem fix the one, then git rebase --continue && git review -R -y
19:53:29 artom Ah, yeah, rebasing on the one below it instead of master isn't a bad idea
19:53:35 artom Tiny bit patronizing tho ;)
19:54:08 mriedem right don't rebase on master
19:54:15 mriedem that makes the diffs between patches hard to follow
19:54:38 mriedem i only rebase on master if i have a merge conflict
19:54:59 artom At some point, someone somewhere will make a diff of diffs tool
19:55:08 artom I remember Diana talking about it before she left Red hat
19:55:17 dansmith git review can do that for ou
19:55:21 mriedem tonyb probably already has one
19:55:39 artom dansmith, yeah? Will need to look into that
19:55:44 dansmith git review -m XXX,4,5
19:56:03 mriedem i'm currently explaining the difference, over text message, between bratwurst, kielbasa and summer sausage
19:56:10 mriedem and can't tell if it's some cruel joke
19:56:29 dansmith git review can't do that
19:56:48 dansmith git review will do anything for love, but it won't ... doo... thaaat.
19:56:51 mriedem git review --sausage-index
19:56:54 dansmith hah
19:57:24 dansmith I think you just did
19:57:38 artom So meta
20:01:03 artom mriedem, so is 'vf_trusted' always going to show in device metadata from now on? And set to True/False?
20:01:22 artom Or it'll only appear if the VF is trusted?
20:01:31 artom (Yeah, I could read, but... :/ )
20:01:44 mriedem he's always putting it in,
20:01:50 mriedem default to False for old devices, which is correct
20:02:04 mriedem the new field shows up if the requested version on the metadata api is new enough
20:02:07 artom I'm cool with whatever, just wanted to be sure
20:02:32 mriedem i tend to like adding new fields like we do in the compute rest api, which is we add the new field always, we don't expose it conditionally based on it's value
20:02:50 dansmith definitely
20:03:10 dansmith not all languages have a __contains__ type operator, which makes checking for the presence before the value annoying
20:09:40 mriedem efried: jaypipes: +W on the mirror patch
20:09:53 efried süsse
20:18:31 openstackgerrit Matt Riedemann proposed openstack/nova master: Skip ServerShowV254Test.test_rebuild_server in cells v1 job https://review.openstack.org/569454
20:21:56 artom mriedem, https://review.openstack.org/#/c/566344/7/nova/tests/unit/test_metadata.py@244
20:22:52 artom we need to include it though, no? We're asserting that the metadata will be equal to those dicts we define, so we need to add vf_trusted=False because we expect the code to generate that
20:23:36 mriedem oh is that the thing we're asserting against in the response?
20:23:39 mriedem if so, then sure
20:23:42 mriedem it wasn't obvious
20:24:04 artom Yeah, it's giving me a headache too
20:24:26 mriedem without reading it yet, is it fair to say the ML thread on review nit picking has itself succumbed to nit picking?
20:25:11 artom Duh
20:25:28 artom Really accessible topic, easy visibility points
20:25:32 artom Even *I* contributed
20:45:05 artom mriedem, need to relocate home (via daycare) will try to finish the metadata thing tonight
20:45:14 artom You east or west coast?
20:45:21 artom (Locality-wise, not rap)
20:45:44 artom Though I guess we already have your +2, it's jaypipes or whoever else we need to care about
20:46:28 jaypipes artom: hmm?
20:47:19 artom jaypipes, oh, apologies, for some reason I was sure mriedem roped you into reviewing https://review.openstack.org/#/q/topic:bp/sriov-trusted-vfs+(status:open+OR+status:merged)
20:47:28 artom He's +2 except for 1 patch with nits
20:47:31 artom Which I'm trying to fix
20:47:45 mriedem artom: MN
20:47:47 mriedem no coast
20:47:58 jaypipes artom: ok. can try tomorrow morning (there's a bunch of stuff ahead of that right now)
20:48:10 artom jaypipes, heh, it wasn't a request, but thank you!
20:48:16 mriedem we can throw it in the runways queue
20:48:23 mriedem sahid should have already done that anyway
20:48:28 mriedem i'll add it
20:48:45 mriedem artom: east coast rap-wise
20:49:00 jaypipes mriedem: definitely west coast.
20:49:04 artom mriedem, ok, you have an hour or so on me in terms of time left in day, you might still be around to +2 it if I succeed in making those damn tests pass
21:12:54 mriedem dansmith: before Kevin_Zheng goes off and adds some backoff looping call logic here, you might want to make sure we're not missing something on the problem for which he's trying to account https://review.openstack.org/#/c/568542/1/nova/compute/manager.py@6222
21:13:02 mriedem basically a race window during abort of a live migration
21:26:30 mriedem hmm, also, on surya's patch, she already has a release note https://review.openstack.org/#/c/567878/5/releasenotes/notes/migration-tool-to-populate-inst.avz-29fed2fe57a9764d.yaml
21:26:37 mriedem so i'm not sure what to add to that in the backport
21:27:54 dansmith mriedem: right which is why I suggested a new one
21:28:01 dansmith mriedem: commented on that live migration abort thing
21:28:29 dansmith I think proper ordering will make that mostly unracy for our purposes without a polling loop
21:35:09 mriedem dansmith: alright this is what i had added, but it overlaps pretty heavily http://paste.openstack.org/show/722381/
21:37:11 dansmith mriedem: I just wanted something that indicates that they have to run them again, if you think what is there now is enough, then that's fine
21:37:19 dansmith I agree your paste adds nothing to what is there,
21:37:28 dansmith I was just thinking about calling it out specifically
21:37:30 dansmith but it's cool
21:38:09 dansmith a couple days ago I was working an internal bug where someone updated the code, reported the fix didn't work
21:38:22 dansmith and turns out they didn't even restart nova-compute to get the new code
21:38:29 dansmith so maybe I'm overly sensitive at the moment
21:39:45 mriedem alright, i'll remove my extra reno in the backport. i figured that most deployment tools will run nova-manage db sync and the online data migrations during an update as well as a major upgrade, since it shouldn't hurt if there is nothing new, but can hurt if you don't run it and there is
21:40:22 mriedem also, thanks for the comments on kevin's patch, makes sense when i read it
21:40:54 openstackgerrit Matt Riedemann proposed openstack/nova stable/queens: Metadata-API fails to retrieve avz for instances created before Pike https://review.openstack.org/571317
21:41:47 dansmith I don't think they do actuall
21:41:50 dansmith fwiw
21:42:48 dansmith at least our process for rolling out updates (not upgrades) is basically just yum
21:43:56 cfriesen yeah, we normally wouldn't re-run online data migrations unless it's specifically highlighted.
21:44:17 cfriesen previous fixes have generally fixed things up on accessing the data, and the online data migrations fixed up ones that hadn't been accessed "live"
21:50:36 mriedem alright, well, meh
21:50:45 mriedem i guess people doing updates can run them if needed,
21:50:53 mriedem otherwise people rolling up to those during FFU can get it
21:52:09 openstackgerrit Merged openstack/nova master: Metadata-API fails to retrieve avz for instances created before Pike https://review.openstack.org/567878
21:52:15 openstackgerrit Merged openstack/nova master: Make nova service-list use scatter-gather routine https://review.openstack.org/568271

Earlier   Later