| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-28 | |||
| 13:04:14 | openstackgerrit | Artom Lifshitz proposed openstack/nova stable/ocata: Test InstanceNotFound handling in 'nova usage' https://review.openstack.org/482219 | |
| 13:09:32 | openstackgerrit | Artom Lifshitz proposed openstack/nova stable/pike: Test InstanceNotFound handling in 'nova usage' https://review.openstack.org/499208 | |
| 13:14:18 | alex_xu | johnthetubaguy: yea, we are work on that together | |
| 13:15:54 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Update the placement deployment instructions https://review.openstack.org/469048 | |
| 13:22:12 | johnthetubaguy | alex_xu: cool, I was just thinking that spec didn't use the flavor extra specs mapping dansmith was talking about at the PTG | |
| 13:22:59 | johnthetubaguy | alex_xu: I added comments on the spec anyways | |
| 13:25:11 | openstackgerrit | OpenStack Proposal Bot proposed openstack/os-vif stable/ocata: Updated from global requirements https://review.openstack.org/490256 | |
| 13:25:18 | alex_xu | johnthetubaguy: yea, I just read that, thanks | |
| 13:29:00 | stephenfin | cdent: Some more comments left on that patch from stuff I missed first go round. After those are addressed, I'm +2 | |
| 13:29:15 | cdent | stephenfin: ✔ | |
| 13:35:21 | mdbooth | stephenfin: https://review.openstack.org/#/c/507488/2/nova/tests/unit/virt/libvirt/test_guest.py | |
| 13:35:21 | openstackgerrit | Alex Xu proposed openstack/nova-specs master: Add trait support in the allocation candidates API https://review.openstack.org/497713 | |
| 13:35:33 | mdbooth | stephenfin: They won't be converted to byte strings? | |
| 13:35:40 | mdbooth | I don't understand. | |
| 13:35:52 | mdbooth | We don't need to convert anything to bytestring. | |
| 13:36:01 | stephenfin | mdbooth: Isn't that what this is doing? https://review.openstack.org/#/c/507488/2/nova/virt/libvirt/guest.py | |
| 13:36:21 | stephenfin | if they're unicode values, convert to bytestrings (str in Python 2.7) | |
| 13:36:23 | openstackgerrit | Bhagyashri Shewale proposed openstack/nova master: Don't create instance backup image if rotation is 0 https://review.openstack.org/409644 | |
| 13:36:37 | mdbooth | Not bytestrings, regular strings | |
| 13:36:43 | mdbooth | But only for python2 | |
| 13:36:59 | openstackgerrit | OpenStack Proposal Bot proposed openstack/os-vif stable/newton: Updated from global requirements https://review.openstack.org/373293 | |
| 13:37:11 | mdbooth | We're not interested in what happens to bytestrings, because we don't expect to receive them | |
| 13:37:56 | mdbooth | Just a sec, lemme dig out the libvirt python bindings again... | |
| 13:38:02 | stephenfin | mdbooth: I'm using the wrong terminology so | |
| 13:38:21 | alex_xu | johnthetubaguy: ^ try to address your comment | |
| 13:38:30 | stephenfin | Whatever the default string type in Python3 is | |
| 13:38:34 | stephenfin | *Python 2 | |
| 13:38:46 | stephenfin | i.e. six.binary_type | |
| 13:39:22 | mdbooth | stephenfin: The default string type in Python3 is unicode | |
| 13:39:32 | mdbooth | Oh, python2 | |
| 13:39:40 | mdbooth | It's not binary type | |
| 13:39:49 | mdbooth | b'foo' != 'foo' | |
| 13:40:12 | stephenfin | mdbooth: Yeah it is, in Python 3 | |
| 13:40:14 | stephenfin | *Python 2 | |
| 13:40:16 | stephenfin | dammit | |
| 13:40:19 | mdbooth | Hehe | |
| 13:40:20 | stephenfin | True | |
| 13:40:20 | stephenfin | >>> b'test' == 'test' | |
| 13:40:29 | mdbooth | Ah, so it is | |
| 13:40:37 | mdbooth | In that case, we're already testing that, no? | |
| 13:40:55 | mdbooth | Because we're testing a bare string | |
| 13:40:55 | stephenfin | In Python 2, 'str' == binary strings. In Python 3, 'str' == unicode strings | |
| 13:41:02 | alex_xu | johnthetubaguy: in the allocation_candidate API, we said the parameter named as 'requires', so I guess the extra spec should be "HW_CPU_X86_AVX=require", not the `required`? | |
| 13:41:18 | stephenfin | mdbooth: Yeah, but you're not testing what passing a unicode string into 'migrate' does in Python 3 | |
| 13:41:21 | stephenfin | *Python 2 | |
| 13:41:34 | johnthetubaguy | alex_xu: yeah, keeping those consistent would be a good idea | |
| 13:41:34 | stephenfin | (I'm working on my laptop keyboard and keep missing 2) | |
| 13:41:41 | mdbooth | stephenfin: Yes we do, because the other test already does that | |
| 13:41:48 | mdbooth | because in python 3 u'foo' == 'foo' | |
| 13:41:51 | mdbooth | And we test 'foo' | |
| 13:41:57 | openstackgerrit | Bhagyashri Shewale proposed openstack/nova master: Don't create instance backup image if rotation is 0 https://review.openstack.org/409644 | |
| 13:42:01 | alex_xu | johnthetubaguy: cool, got it | |
| 13:42:42 | mdbooth | stephenfin: So I actually wrote the test, and I changed 'v1' to be u'v1' | |
| 13:42:49 | mdbooth | And asserted that's what we ended up with | |
| 13:42:58 | mdbooth | Then I realised that they're the same thing | |
| 13:43:03 | mdbooth | So actually I hadn't changed anything | |
| 13:43:28 | stephenfin | mdbooth: But what about the other way round | |
| 13:43:42 | stephenfin | If you called 'self.guest.migrate' with a "u'test'" param value | |
| 13:43:58 | mdbooth | test_migrate_v3 already tests that | |
| 13:44:01 | stephenfin | I'd expect the call to 'self.domain.migrateToURI3' to be called with "b'test'" | |
| 13:44:05 | stephenfin | But only for Python 2 | |
| 13:44:18 | mdbooth | test_migrate_v3_unicode tests that | |
| 13:44:32 | mdbooth | Because it passes u'v1' and asserts 'v1' | |
| 13:44:36 | stephenfin | Oh FFS | |
| 13:44:46 | stephenfin | I read 'not six.PY2' as 'not six.PY3' | |
| 13:44:57 | stephenfin | and thought that test was only for Python 3. Sorry! | |
| 13:44:59 | mdbooth | HAHA, sorry! | |
| 13:45:14 | mdbooth | -ESTUPIDNEGATIONS | |
| 13:45:33 | stephenfin | :D | |
| 13:45:54 | stephenfin | mdbooth: OK, let me give that one more spin through so. Sorry for the confusion :) | |
| 13:46:02 | mdbooth | NP, thanks again | |
| 13:47:21 | mdbooth | FWIW I wrote it that way because I way being anal. | |
| 13:47:50 | mriedem | https://review.openstack.org/#/c/507938/ needs final +2, regression in pike | |
| 13:48:14 | openstackgerrit | OpenStack Proposal Bot proposed openstack/os-vif stable/newton: Updated from global requirements https://review.openstack.org/373293 | |
| 13:48:51 | mdbooth | mriedem: https://review.openstack.org/#/c/507202/ libvirt data corruptor since forever | |
| 13:52:48 | mriedem | mdbooth: ok i'm going to put a patch on top of that to test live block migration with something like this https://review.openstack.org/#/c/481290/ | |
| 13:53:25 | mdbooth | mriedem: kk | |
| 13:54:00 | kashyap | mriedem: Oh cool. Funnily enough, I was just checking an hour ago if we do (non-shared) live block migration in the Gate | |
| 13:54:17 | mdbooth | Although the bug itself is really confined to the calling conventions of migrateToURI3 () | |
| 13:54:52 | mdbooth | We're going to have to backport it a long way, btw. | |
| 13:56:51 | openstackgerrit | Matt Riedemann proposed openstack/nova master: DNM: Run test_volume_backed_live_migration and iscsi test https://review.openstack.org/508163 | |
| 13:56:55 | mriedem | ^ | |
| 13:57:42 | kashyap | Hmm, as an aside, seems like Nova cgit is out of sync with GitHub: | |
| 13:57:45 | kashyap | https://github.com/openstack/nova/blob/master/nova/tests/live_migration/hooks/run_tests.sh | |
| 13:57:48 | kashyap | http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/live_migration/hooks/run_tests.sh?id=068d851 | |
| 13:58:03 | kashyap | Maybe /me should check w/ -infra | |
| 13:58:15 | mriedem | mdbooth: if you plan on backporting this to essex, do we really need https://review.openstack.org/#/c/507488/ in the mix for the backports? | |
| 13:58:23 | mriedem | you can't isolate that for your fix and backport, and then do ^ on top? | |
| 13:58:54 | mdbooth | mriedem: I'll do that during the backport. IIRC it just poked a bug in a unit test. | |
| 13:58:55 | dansmith | bauzas: wanna re-+W this after fixes were applied? | |
| 13:58:56 | dansmith | https://review.openstack.org/#/c/498947/10 | |
| 13:59:07 | mriedem | mdbooth: why not do it now? | |
| 13:59:16 | mriedem | otherwise the stable reviewers have to figure out why there is a diff | |
| 13:59:33 | mdbooth | Because it makes it messier upstream, and it's like a 1 line diff. | |
| 13:59:38 | mdbooth | In a unit test. | |
| 13:59:54 | mriedem | mdbooth: so when you say backport, you only mean internal backports? | |
| 14:00:09 | mdbooth | I'll be doing both. | |
| 14:00:38 | mriedem | ok. so as a stable reviewer, why do i need to sort out this diff? just isolate the py3 thing into the fix patch for migrateToURI and do the more general thing on top | |
| 14:01:16 | mdbooth | You should read my backport commit messages :) They're immaculate. | |
| 14:02:07 | dansmith | -1 | |