Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-01-04
16:38:57 edleafe cdent: you mean protecting against negative used values?
16:39:24 cdent yes, and also being explicit about the conditional being numeric
16:39:48 edleafe cdent: not sure I follow
16:40:39 cdent I just think that self.used == 0 or self.used <= 0 is more explicit/readable than not self.used
16:41:02 mriedem sdague: mtreinish: easy one https://review.openstack.org/#/c/416401/
16:41:06 mriedem sfinucan: thanks
16:41:09 edleafe cdent: self.used == 0 is major ugly in Python
16:41:27 edleafe <= 0 is another thing
16:41:31 mtreinish mriedem: +2
16:41:45 cdent but it is _exactly_ what you mean to be testing for there, especially based on the message raised if it is true
16:42:00 edleafe cdent: if you think we need to guard against negative values, I'll add <= 0
16:42:40 cdent a) just out of curiosity why is self.used == 0 ugly? b) I'm not sure if we do or not, I'm asking, not telling
16:43:07 mriedem dansmith: there will be a thrilling cells v2 meeting in 17 minutes yes?
16:43:34 mriedem dare i say titilating
16:44:09 edleafe cdent: 0, False, empty string/dict/list/tuple/set/etc are all boolean False, so you don't test for == 0, or == {}, or the like
16:44:43 cdent edleafe: I understand that, and that's my point: you _are_ testing for 0 here. Not False or empty
16:45:03 openstackgerrit Sylvain Bauza proposed openstack/nova: Expose a REST API for a specific list of RPs https://review.openstack.org/392569
16:45:06 cdent because the fields definition has already established that used must be an int
16:45:10 dansmith mriedem: figured yeah
16:45:17 edleafe cdent: if you are getting unexpected parameter types here, you have much bigger problems
16:45:46 bauzas jaypipes: mriedem: dansmith: others, wanna bite https://review.openstack.org/392569 ?
16:46:07 cdent edleafe: hmm, I think we are miscommunicating a bit. At what stage of the process are ovo field types validated?
16:46:56 mriedem bauzas: it's in a tab
16:47:05 cdent if it is _before_ create is called, then self.used is always an int, and therefore the most limited and explicit (and thus readable) conditional is self.used == 0
16:47:17 bauzas mriedem: just added a new test, so Ctrl-R is needed :)
16:47:21 cdent edleafe: that's all I'm saying
16:47:45 mriedem tdurakov: pkoniszewski: it would be awesome if we had a diagram of the call stack during live migration so i didn't have to relearn it everytime i'm reviewing something like this https://review.openstack.org/#/c/140045/51/nova/virt/hyperv/livemigrationops.py
16:47:53 edleafe cdent: if it could be an unexpected type, then you should do the type checking sepately
16:48:11 cdent yes, we agree on that
16:48:38 edleafe cdent: but if it's the correct type, then == 0 is ugly
16:48:45 cdent no, it's explicit
16:49:13 sdague_ mriedem: +A
16:49:45 cdent edleafe: but since that's aesthetics, I guess we should really just try to answer if we need to worry about negative numbers. I can't remember the answer to that.
16:49:56 bauzas cdent: edleafe: HTH https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L70-L72
16:50:52 edleafe bauzas: so it's enforced when the value is set, not when it's saved
16:51:03 bauzas indeed
16:51:09 bauzas easy to test
16:51:26 openstackgerrit Ed Leafe proposed openstack/nova: Do not post allocations that are zero https://review.openstack.org/407180
16:51:36 edleafe cdent: ^^ try again :)
16:52:00 cdent now reason isn't perfect :)
16:53:35 bauzas cdent: edleafe: FWIW http://paste.openstack.org/show/593884/
16:54:12 edleafe bauzas: thx
16:54:21 edleafe cdent: what's wrong with the reason??
16:54:34 cdent "allocations of 0 resources used not allowed'"
16:54:46 cdent doesn't account for the negative handling you've just added
16:54:55 cdent but the :) was there for a reason
16:55:11 cdent I don't really care, but if we're striving for perfection...
16:55:21 edleafe cdent: ah, you meant in the error messag
16:55:30 edleafe I thought you meant the comment that I changed
16:57:04 openstackgerrit Ed Leafe proposed openstack/nova: Do not post allocations that are zero https://review.openstack.org/407180
16:57:13 edleafe cdent: and you know I can't leave something like that there. ^^
16:57:27 cdent I have become your enabler
17:00:37 mdbooth Could we please bang out these final 3 py3 fixes for libvirt: https://review.openstack.org/#/c/403423/ https://review.openstack.org/#/c/403419/ https://review.openstack.org/#/c/403431/
17:00:46 mdbooth They've all had multiple rounds of review and rebases
17:01:09 raj_singh sfinucan: https://review.openstack.org/#/c/397774/ looks good to me, but because I fixed bunch of stuff in this I want johnthetubaguy to review it.
17:01:16 mdbooth It would be nice to get these done and forget about them
17:02:09 sfinucan mdbooth: Oops, forgot about those. I'll take a look shortly
17:02:32 mdbooth sfinucan: Thanks. Looks like you commented on the first on Monday?
17:02:41 sfinucan raj_singh: Aye, sounds fair. You know more about that than me though, so a +1 wouldn't go astray
17:03:00 sfinucan mdbooth: I did, but I'd lost the context. Not going to hold it up for my one comment
17:03:22 mdbooth Good, because I don't know the answer :)
17:06:50 mdbooth sfinucan: They mutually conflict (in tests-py3.txt), btw, so at least one of them will have to be remunged whatever.
17:06:59 edleafe cdent: did you mean 'persevering' or 'preseverating'? :)
17:07:18 mdbooth preseverating sounds wonderful
17:07:31 cdent heh, well played
17:07:32 mdbooth and potentially dangerous
17:07:57 cdent how about both?
17:14:54 openstackgerrit Rafael Folco proposed openstack/nova: DELETE all inventory for a resource provider https://review.openstack.org/416669
17:16:54 openstackgerrit Dan Peschman proposed openstack/nova: No API cell up-call to delete consoleauth tokens https://review.openstack.org/415922
17:22:43 mriedem woot https://blueprints.launchpad.net/nova/+spec/consistent-query-parameters-validation is complete
17:22:50 mriedem alex_xu: nice work
17:25:58 openstackgerrit Chris Dent proposed openstack/nova: [placement] Add a bit about extraction plans to placement_dev https://review.openstack.org/416676
17:27:44 openstackgerrit Rafael Folco proposed openstack/nova: DELETE all inventory for a resource provider https://review.openstack.org/416669
17:28:50 cdent mriedem: is this still on your radar? https://review.openstack.org/#/c/414230/ (more logging on rp update failures)
17:29:09 cdent If not, I can probably pick it up
17:29:26 mriedem cdent: i'm not actively on it right now so feel free
17:29:38 openstackgerrit Gábor Antal proposed openstack/nova: Transform instance.volume_attach notification https://review.openstack.org/401992
17:29:42 cdent ✔
17:30:57 openstackgerrit Rafael Folco proposed openstack/nova: DELETE all inventory for a resource provider https://review.openstack.org/416669
17:33:14 openstackgerrit Rafael Folco proposed openstack/nova: DELETE all inventory for a resource provider https://review.openstack.org/416669
17:35:47 openstackgerrit Chris Dent proposed openstack/nova: WIP: Add more debug logging on RP inventory update failures https://review.openstack.org/414230
17:36:15 openstackgerrit Gábor Antal proposed openstack/nova: Transform instance.volume_detach notification https://review.openstack.org/408676
17:37:01 openstackgerrit Gleb Stepanov proposed openstack/python-novaclient: Refactor novaclient code https://review.openstack.org/416682
17:39:17 sfinucan mdbooth: Done (all trivial reapprovals bar the first). You just need to rope someone like japyipes in now :)
17:44:32 openstackgerrit Balazs Gibizer proposed openstack/nova: Transform aggregate.create notification https://review.openstack.org/394511
17:45:13 openstackgerrit Balazs Gibizer proposed openstack/nova: Transform aggregate.delete notification https://review.openstack.org/394512
17:45:40 openstackgerrit Balazs Gibizer proposed openstack/nova: Add aggregate notification related enum values https://review.openstack.org/396209
17:45:53 openstackgerrit Balazs Gibizer proposed openstack/nova: Transform aggregate.add_host notification https://review.openstack.org/396210
17:46:04 openstackgerrit Balazs Gibizer proposed openstack/nova: Transform aggregate.remove_host notification https://review.openstack.org/396211
17:53:05 johnthetubaguy gibi: I wasn't sure about this one, I am curious about your thoughts around that: https://review.openstack.org/#/c/408689/
18:09:46 sean-k-mooney mriedem: QQ when https://review.openstack.org/#/c/411988/ is merged to relese the new version of os-vif will it automatically update the requirements repo or should i submit a patch to the requiremets repo to do that?
18:10:48 openstackgerrit Dan Smith proposed openstack/nova: Multicell support for instance listing https://review.openstack.org/396775
18:10:48 openstackgerrit Dan Smith proposed openstack/nova: Fix non-parameterized service id in hypervisors sample tests https://review.openstack.org/410471
18:10:49 openstackgerrit Dan Smith proposed openstack/nova: Make the SingleCellSimple fixture a little more comprehensive https://review.openstack.org/410472
18:10:49 openstackgerrit Dan Smith proposed openstack/nova: WIP: Make api_samples tests use simple cell environment https://review.openstack.org/410473
18:10:53 openstackgerrit Dan Smith proposed openstack/nova: WIP Move instance creation to conductor https://review.openstack.org/319379
18:11:40 mriedem sean-k-mooney: you have to submit a patch to raise the minimum required version of os-vif in global-requirements yourself
18:12:02 mriedem the upper-constraints update happens automatically, but if you're going to bump g-r you might as well bump u-c in the same patch - i think you'd have to anyway for the tests to pass
18:13:37 sean-k-mooney mriedem: thanks. im going to follow up with sfinucan again tomorow and see if we can get his outstanding patch merged so that the release can go ahead.
18:14:06 sean-k-mooney ill update the patche to the release repo and to the requirements repo once that is done

Earlier   Later