| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-17 | |||
| 11:13:01 | trinaths | jaypipes: agree. | |
| 11:33:59 | openstackgerrit | Rodolfo Alonso Hernandez proposed openstack/os-vif master: Add __str__ method to Host* objects https://review.openstack.org/493082 | |
| 11:37:46 | openstackgerrit | Michael Still proposed openstack/nova master: Cleanup mount / umount and associated rmdir calls https://review.openstack.org/494423 | |
| 12:21:21 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 12:23:34 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 13:39:51 | maciejjozefczyk | dansmith: Hello, could you take a look on this https://review.openstack.org/#/c/491808/ ? | |
| 13:51:26 | dansmith | maciejjozefczyk: I think you're missing a test case there, comments in the review | |
| 14:08:46 | maciejjozefczyk | dansmith: I dont get your comment. You mean to add asserting that fileters dictonary is dict with specific key set according to test case? | |
| 14:10:26 | dansmith | maciejjozefczyk: you need to assert that the filters dict passed to get_by_filters() looks like you expect and includes the host value you are changing (moving) | |
| 14:10:47 | dansmith | admittedly the test doesn't do a good job of this now, but given that you're moving the code you should make sure you're covered | |
| 14:11:14 | dansmith | because before it was asserting that host was in filters as passed to the _get_instances_on_driver() method, which can no longer be asserted | |
| 14:23:04 | dansmith | maciejjozefczyk: does that make sense? if necessary I can just pull it down and fix it myself | |
| 14:26:47 | maciejjozefczyk | dansmith: I'm back, give me a sec | |
| 14:30:56 | maciejjozefczyk | dansmith: so in test_get_instances_on_driver_fallback() It could be better to add little asserting that filters are the same like in L1451? I thought its already done while checking mock_instance_list mock, but I can add assertEqual too | |
| 14:31:12 | maciejjozefczyk | dansmith: you mean to solve it this way or I'm wrong too? | |
| 14:31:59 | dansmith | maciejjozefczyk: L1451 isn't an assertion | |
| 14:34:08 | dansmith | maciejjozefczyk: your assertion on L1487 is asserting that the thing you passed to _get_instances_on_driver() (which modifies it) is the thing that gets passed to get_by_filters(), but you don't assert what is in it | |
| 14:43:10 | maciejjozefczyk | dansmith: Yhmmm, right... | |
| 14:43:59 | dansmith | maciejjozefczyk: I think this is what you want: https://pastebin.com/egQnq8cH | |
| 14:44:18 | maciejjozefczyk | dansmith: what do you think about remove host key from filters in line 1452, and then asserting that it has been added after _get_instances_on_driver ? | |
| 14:44:49 | maciejjozefczyk | yes, exactly | |
| 14:45:05 | dansmith | maciejjozefczyk: asserting that it has been added to the thing you passed isn't good, but asserting that it was passed to the query method is, yes | |
| 14:45:24 | maciejjozefczyk | dansmith: okey | |
| 14:48:52 | maciejjozefczyk | dansmith: Thanks a lot for you time, sorry that I'm so retarted today | |
| 14:49:08 | dansmith | maciejjozefczyk: no problem :) | |
| 14:52:45 | gibi | dansmith: hi! Could you check the bugfix for the missing cleanup at evacuation https://review.openstack.org/#/c/493037/ ? Jay is +2 about it. | |
| 14:58:04 | dansmith | gibi: this is another thing we need for pike I guess? | |
| 14:58:15 | dansmith | sorry I hadn't noticed this yet | |
| 14:58:36 | cdent | gibi: why is remove_provider… what is wanted there? is the instance destroyed globally, or just on the compute node? | |
| 14:59:06 | cdent | “evacuate” is a very confusing term | |
| 14:59:54 | cdent | ah, I see from the tests, we want some allocations left over | |
| 15:00:23 | gibi | dansmith: I guess this is something that worked in Ocata but doesn't in Pike RC1 so yes we need a backport | |
| 15:00:50 | gibi | cdent: we want to remove the allocation from the source host of the allocation | |
| 15:00:57 | gibi | cdent: we want to remove the allocation from the source host of the evacuation | |
| 15:01:17 | gibi | cdent: so the source host will be the removed provider | |
| 15:01:47 | cdent | gibi: yeah, I figured that out after looking at the tests. I was struggling to remember the meaning of evacuate | |
| 15:02:09 | gibi | cdent: I agree that evacuate should be renamed to recreate | |
| 15:02:22 | gibi | dansmith: we have a similar bug in shelve offload https://bugs.launchpad.net/nova/+bug/1710249 | |
| 15:02:23 | openstack | Launchpad bug 1710249 in OpenStack Compute (nova) "nova doesn't clean up the resources after shelve offload" [High,In progress] - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 15:02:34 | dansmith | gdi gibi stop finding bug! :) | |
| 15:02:36 | dansmith | *bugs | |
| 15:03:08 | gibi | dansmith: I don't want to make you mad but I'm currently looking at soft delete + periodic reclaim and that seems buggy as well... | |
| 15:03:15 | cdent | woot! | |
| 15:03:25 | dansmith | gibi: nooooo :P | |
| 15:04:20 | gibi | dansmith: but on the plus side simple migrate confirm / revert works based on https://review.openstack.org/#/c/493865/ | |
| 15:04:29 | dansmith | that's cool | |
| 15:04:48 | openstackgerrit | Maciej Jozefczyk proposed openstack/nova master: Remove host filter for _cleanup_running_deleted_instances periodic task https://review.openstack.org/491808 | |
| 15:06:16 | gibi | dansmith: but I will be on vacation for a week starting at next Tuesday so my bug flow will decrease ;) | |
| 15:08:24 | dansmith | gibi: good :) | |
| 15:08:30 | dansmith | gibi: can you propose that against pike? | |
| 15:08:50 | cdent | gibi: I rebased matt’s https://review.openstack.org/#/c/490733/ yesterday, and because of all your bug finding and fixing it needs a pretty manual rebase, but once we finally get going with shared providers, it will be handy | |
| 15:09:48 | gibi | dansmith: do you mean the evacuate one or both the evac and the shelve offload patches? | |
| 15:10:00 | dansmith | jaypipes: still around? | |
| 15:10:37 | dansmith | gibi: the evacuate one for now since it's on the way to the gate. I'm looking at the shelve one now | |
| 15:10:48 | gibi | cdent: ack, I will review that | |
| 15:11:03 | gibi | dansmith: OK | |
| 15:12:08 | jaypipes | dansmith: yyup | |
| 15:12:10 | maciejjozefczyk | dansmith: I've updated https://review.openstack.org/#/c/491808 ; looks good now? | |
| 15:12:29 | dansmith | jaypipes: can you go over this one too? I'm doing so as we speak: https://review.openstack.org/#/c/493834 | |
| 15:12:37 | dansmith | maciejjozefczyk: will look in a sec | |
| 15:13:01 | jaypipes | dansmith: ah, yeah, I owed gibi a re-review on that one. doing it now. | |
| 15:13:32 | dansmith | jaypipes: pretty sure you owe gibi a kidney or something by now, but.. yeah thanks | |
| 15:13:57 | jaypipes | dansmith: indeed :) | |
| 15:16:17 | gibi | I have two health kindeys but maybe we can freeze them for later use :) | |
| 15:16:55 | dansmith | gibi: now, but later you may need one unexpectedly | |
| 15:17:15 | dansmith | gibi: or did you mean freeze the one jaypipes owes you? | |
| 15:17:30 | dansmith | because yeah, another ten years of crunchy bars and his'll be useless | |
| 15:17:34 | dansmith | so good idea to freeze now | |
| 15:17:43 | gibi | dansmith: yeah, exaxtly | |
| 15:17:59 | dansmith | good call | |
| 15:19:19 | gibi | English is hard | |
| 15:19:48 | jaypipes | dansmith, gibi: k, +2 from me on that one. | |
| 15:19:59 | dansmith | heh | |
| 15:20:09 | jaypipes | the patch, not the freezing part :) | |
| 15:20:15 | dansmith | jaypipes: got one more for you in just a sec | |
| 15:20:32 | jaypipes | k | |
| 15:20:46 | dansmith | jaypipes: https://review.openstack.org/#/c/491808 | |
| 15:20:58 | dansmith | jaypipes: you were +2, I just had some comments on test coverage, but looks good to me now | |
| 15:21:13 | dansmith | maciejjozefczyk: I'm assuming we should put that into pike as well | |
| 15:21:33 | jaypipes | ah, maciejjozefczyk and dpawlik's patch | |
| 15:21:54 | cdent | I’m in a quandry: If I want to keep jay’s kidneys healthy, for the sake of gibi, then I shouldn’t deliver crunchie bars. | |
| 15:22:15 | dansmith | heh | |
| 15:22:23 | gibi | cdent: or, we have to freeze that kindey before you deliver | |
| 15:22:32 | jaypipes | dansmith: k, +Wallaby'd maciejjozefczyk's patch. | |
| 15:22:54 | dansmith | thanks | |
| 15:23:09 | dansmith | gibi: +W on the shelve patch, so please propose that for pike too | |
| 15:23:24 | gibi | dansmith: thank. I will do it | |
| 15:23:28 | maciejjozefczyk | jaypipes: dansmith thx | |
| 15:24:21 | dansmith | maciejjozefczyk: you too for pike | |
| 15:24:48 | gibi | dansmith: what is the policy? Only propose the bugfix on stable or both the functional test and the bugfix in two separate patches or maybe squash them? | |
| 15:25:14 | dansmith | gibi: never squash unless you have to, | |
| 15:25:25 | dansmith | gibi: but are you talking about your "replace chance" patch? | |
| 15:26:09 | gibi | dansmith: nope. both the evac and the offload fix consist of two patches one for the functional test and one for the bugfix | |
| 15:26:31 | dansmith | gibi: oh and the functional test are already in tree, right? | |
| 15:26:49 | gibi | dansmith: for the shelve it is still on review https://review.openstack.org/#/c/493062/ | |
| 15:27:02 | gibi | dansmith: the evac test is in the master but not on stable pike | |
| 15:27:04 | dansmith | oh heh, I see now, I was confusing | |
| 15:27:21 | dansmith | gibi: for both just backport all the patches as they are | |
| 15:27:36 | gibi | dansmith: OK | |
| 15:28:00 | dansmith | gibi: I was looking at that shelve test in the shelve fix and thinking "oh I didn't see this go in, but this is nice" | |