| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-28 | |||
| 10:01:28 | lyarwood | artom: hey yeah | |
| 10:01:32 | lyarwood | artom: ack looking | |
| 10:35:52 | openstackgerrit | Zhenyu Zheng proposed openstack/nova master: nova-manage db archive_deleted_rows is not multi-cell aware https://review.openstack.org/507486 | |
| 11:17:57 | openstackgerrit | John Garbutt proposed openstack/nova master: WIP: Send traits to ironic on server boot https://review.openstack.org/508116 | |
| 11:25:20 | openstackgerrit | Chris Dent proposed openstack/nova master: Do not monkey patch eventlet in unit tests https://review.openstack.org/507923 | |
| 11:25:21 | openstackgerrit | Chris Dent proposed openstack/nova master: Do not setup conductor in BaseAPITestCase https://review.openstack.org/508120 | |
| 11:25:21 | openstackgerrit | Chris Dent proposed openstack/nova master: DNM: Don't monkey patch eventlet in functional https://review.openstack.org/506668 | |
| 11:33:46 | mdbooth | cdent: Remind me, were you proposing removing eventlet from tests? | |
| 11:33:57 | cdent | I was yes | |
| 11:34:18 | cdent | there are monkey_patch calls at the top of the unit and functional trees | |
| 11:34:26 | cdent | removing the one at the top of unit is no problem | |
| 11:34:39 | cdent | the one at the top of functional is a problem, but limited | |
| 11:34:59 | mdbooth | cdent: Data point: I've used eventlet in tests explicitly a couple of times because its locking primitives can be killed with Ctrl-C, whereas the regular python ones can't. | |
| 11:35:14 | cdent | used explicitly: great | |
| 11:35:23 | mdbooth | This has no impact on the test usually, but is really useful when you're debugging it | |
| 11:35:25 | cdent | it’s the global monkey patching that i think is bad news | |
| 11:36:13 | mdbooth | Cool, just thought I'd bring it up | |
| 11:36:26 | cdent | yeah, thanks. | |
| 11:37:09 | cdent | In my digging around it looks like there’s still a fair amount of tests under unit that ought to be under functional, but I’m pretty sure I don’t want to fall in that hole (yet) | |
| 11:40:26 | cdent | yeah, I grepped for ‘start_service’ as sort of an initial feel around and the boundary has been permeated | |
| 11:45:35 | cdent | johnthetubaguy, stephenfin : can you guys weigh in on https://review.openstack.org/#/c/469048/ it’s some placement docs that have been languishing for a long time | |
| 11:46:16 | cdent | and this is a test that’s also been languishing, adds a bit more coverage: https://review.openstack.org/#/c/485209/ | |
| 11:46:37 | cdent | gibi: you too on both of those | |
| 11:51:31 | openstackgerrit | John Garbutt proposed openstack/nova master: WIP: Send traits to ironic on server boot https://review.openstack.org/508116 | |
| 12:05:23 | sdague | ok, the qemu >= 2.10 patch should really really be ready this time - https://review.openstack.org/#/c/505673/ - stephenfin, bauzas, johnthetubaguy if anyone wants to take a quick look | |
| 12:10:04 | mdbooth | Any chance of some eyes on this: https://review.openstack.org/#/c/507202/ | |
| 12:10:20 | mdbooth | It's a data corruptor with no current mitigation other than "don't do that" | |
| 12:10:57 | mdbooth | The dependent patch is an annoying python 3-ism | |
| 12:11:46 | mdbooth | That's unfair, it's a bug in the libvirt python bindings, and a questionable default choice in the lxml python bindings | |
| 12:20:14 | cdent | mdbooth: that etree default stuff has bit me so many times | |
| 12:20:24 | cdent | (not in an openstack context, but elsewhere) | |
| 12:21:10 | cdent | sdague: no one will believe you until you use a 3rd “really” | |
| 12:21:29 | mdbooth | cdent: It bit me, so I did my usual trick of fixing the whole problem rather than just my bit. Ironically, that decision usually bites me. | |
| 12:39:07 | stephenfin | mdbooth: Comments left. One of us is missing something :) | |
| 12:39:24 | mdbooth | stephenfin: Thanks. Look in a bit. | |
| 12:48:39 | openstackgerrit | konstantin proposed openstack/nova master: switch from filesystem to disk for parallels containers https://review.openstack.org/506687 | |
| 12:48:40 | openstackgerrit | konstantin proposed openstack/nova master: don't add device address if there is no any units https://review.openstack.org/506686 | |
| 12:54:55 | openstackgerrit | OpenStack Proposal Bot proposed openstack/os-vif stable/ocata: Updated from global requirements https://review.openstack.org/490256 | |
| 12:57:05 | cdent | thanks stephenfin for the robust review | |
| 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 | |