| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-01 | |||
| 01:01:36 | jaypipes | heh | |
| 01:02:08 | mriedem | jaypipes: agree on the naming there, we could rename / refactor later too | |
| 01:02:25 | mriedem | plus there is a lot of redundant code in there before we get to the confirm/revert action which could be put into a private common method | |
| 01:02:51 | jaypipes | mriedem: honestly, I just don't understand why there's a need for the whole "do this test in the opposite order" thing. | |
| 01:04:59 | dansmith | jaypipes: because you fail it differently in each case | |
| 01:05:09 | dansmith | jaypipes: keep it on master and disable the skips and you'll see | |
| 01:05:26 | jaypipes | dansmith: but if the whole "availability_zone": "nova:" + initial thing is so deterministic, why the need? | |
| 01:05:38 | dansmith | because going from one compute node to the other, and running their periodics in that order end up with an updated allocation | |
| 01:05:45 | dansmith | in the reverse order, one of them deletes it last | |
| 01:06:00 | dansmith | i.e. one updates, then the other deletes | |
| 01:06:06 | dansmith | vs one deletes then the other updates (creates) | |
| 01:06:45 | jaypipes | with all the stuff we discussed today about ocata computes, there's going to need to be so many friggin conditionals in these code paths that we're introducing more risk than anything else :( | |
| 01:07:00 | dansmith | you mean in order to pass the tests? | |
| 01:07:07 | dansmith | that's exactly why we must have it tested both ways | |
| 01:07:20 | dansmith | ocata to pike _has_ to work, and pike to pike _has_ to work, with no leaks | |
| 01:07:35 | jaypipes | well these tests don't test ocata to pike, only pike to pike | |
| 01:07:38 | dansmith | pike to ocata I can kind of get on board with allowing to be leaky, but it's still not awesome | |
| 01:07:50 | dansmith | jaypipes: exactly, we must have _at least_ this much coverage, | |
| 01:07:59 | dansmith | and we really need one that simulates the pike/ocata split | |
| 01:08:38 | dansmith | we have the grenade multinode case, which helps, but it's not deterministic | |
| 01:08:49 | dansmith | and I think we've shown thus far, that if we don't lay down a test-driven approach to what we expect, | |
| 01:08:55 | jaypipes | I'm in a fix one thing, break ten others place right now :( | |
| 01:08:58 | dansmith | we're going to keep bouncing around between partial solutions | |
| 01:10:23 | dansmith | jaypipes: I'm not sure what the alternative is.. if I can write a test that shows that the code is broken if we end up running things in a particular sequence, that's a problem right? | |
| 01:11:21 | mriedem | we can possibly build on this to make one of the computes' service version be ocata | |
| 01:11:31 | mriedem | to test that code path in the change that checks the min service version | |
| 01:11:32 | jaypipes | dansmith: I'm not disagreeing with you that shit is broken. I just don't know how to fix this without breaking the happy path | |
| 01:11:44 | dansmith | mriedem: we have to simulate the ocata way of managing allocations too | |
| 01:11:46 | mriedem | plus we can do the single node resize to same host testing building on this | |
| 01:12:15 | dansmith | mriedem: i.e. the "always blow away everything without looking first" way | |
| 01:12:18 | mriedem | happy path is all computes are pike isn't it? | |
| 01:12:22 | dansmith | yes | |
| 01:12:27 | mriedem | which this tests | |
| 01:12:30 | dansmith | right | |
| 01:12:42 | jaypipes | no. happy path is non-move operations. | |
| 01:12:49 | dansmith | hah | |
| 01:12:56 | dansmith | that's the beer path | |
| 01:13:10 | mriedem | so, | |
| 01:13:26 | mriedem | what about bfv with shared storage resize to another az? | |
| 01:13:29 | mriedem | will THAT work?! | |
| 01:13:45 | jaypipes | not funny | |
| 01:13:48 | mriedem | uh mixed compute too | |
| 01:14:16 | mriedem | fine fine fine | |
| 01:14:19 | mriedem | s/resize/evacuate/ | |
| 01:14:23 | mriedem | happy? | |
| 01:15:35 | mriedem | so i think the tests as written are a same-version compute good representation of what we expect for confirm and revert, and before dan moved them from the top of the series of fixes, they were passing | |
| 01:15:45 | mriedem | so i think we get that baseline going, and then starting adding in test wrinkles | |
| 01:16:00 | mriedem | i think this is all easier once we have the tests written | |
| 01:16:21 | dansmith | mriedem: meaning they worked on top of the series, which means the series can work atop the tests, right? | |
| 01:16:30 | mriedem | yeah | |
| 01:16:39 | dansmith | that was my reason for doing that before rebasing, to make sure they could work with jay's set | |
| 01:16:49 | mriedem | just like how we do the functional regression bug testaroos | |
| 01:17:05 | dansmith | agreed, we *have* to get this baseline working, and then we can expand the base a bit | |
| 01:17:12 | dansmith | for things like single compute at least | |
| 01:17:31 | jaypipes | I don't disagree with you. | |
| 01:17:38 | jaypipes | just raging that's all. | |
| 01:17:43 | jaypipes | I'll figure it out eventually. | |
| 01:18:47 | mriedem | i'll go back to commenting up dan's cells docs patch :) | |
| 01:27:30 | jaypipes | dansmith: still here? | |
| 01:27:35 | dansmith | jaypipes: yeah | |
| 01:27:45 | jaypipes | dansmith: so this: | |
| 01:27:45 | jaypipes | 1136 compute_version = objects.Service.get_minimum_version( | |
| 01:27:46 | jaypipes | 1137 context, 'nova-compute') | |
| 01:27:46 | jaypipes | 1138 heal_allocations = False | |
| 01:27:46 | jaypipes | 1139 if compute_version < 22: | |
| 01:27:46 | jaypipes | 1140 heal_allocations = True | |
| 01:27:56 | jaypipes | is not working | |
| 01:28:09 | jaypipes | or at least, it's returning heal_allocations = True in the functional tests | |
| 01:28:27 | dansmith | jaypipes: are you using the allservicescurrent fixture? | |
| 01:28:28 | jaypipes | which is triggering the healing of allocations improperly. | |
| 01:28:36 | dansmith | otherwise you probably have a min ver of zero | |
| 01:28:38 | jaypipes | dansmith: sigh | |
| 01:28:47 | dansmith | jaypipes: mriedem pointed that out earlier | |
| 01:28:50 | jaypipes | you even told me about that earlier. | |
| 01:28:54 | jaypipes | yeah.. :( | |
| 01:28:56 | dansmith | yeah, sorry | |
| 01:39:57 | mriedem | dansmith: ok comments in https://review.openstack.org/#/c/487183/ for the cells docs - looks real purdy | |
| 01:40:06 | mriedem | you can tell me you'll look in the morning and i won't be hurt | |
| 01:40:31 | dansmith | mriedem: I'll look in the morning | |
| 01:41:59 | dansmith | but.. you said... | |
| 01:46:21 | jaypipes | dansmith: other_provider_uuid is the source host right? | |
| 01:46:40 | dansmith | jaypipes: it depends on which direction you're going | |
| 01:47:12 | dansmith | jaypipes: actually, it should always be target the way it's written I think | |
| 01:47:25 | dansmith | provider_uuid is instance[host] and other is the target right/ | |
| 01:47:26 | jaypipes | dansmith: ? if "host" is always the one you were originally scheduled to, then other is the one you're going *to*, right? | |
| 01:47:57 | jaypipes | yeah, ok | |
| 01:48:33 | jaypipes | dansmith: the only thing the "other direction" refers to isn't the direction but rather the order in which the hosts' update_available_resource() is called. | |
| 01:48:45 | jaypipes | I think? | |
| 01:48:46 | dansmith | no | |
| 01:48:54 | dansmith | we always call host1 first and host2 second | |
| 01:49:00 | dansmith | for the periodic | |
| 01:50:47 | mriedem | other_provider_uuid is the target | |
| 01:50:55 | dansmith | right | |
| 01:51:00 | dansmith | the destination host | |
| 01:51:06 | mriedem | we could rename to target or dest or whatever | |
| 01:51:13 | dansmith | yep | |
| 01:51:45 | jaypipes | I gotta look at this in the morning. I'm fried right now :( | |
| 01:52:06 | dansmith | jaypipes: you want me to convert it to source/dest so it's like that for you in the morning? | |
| 01:52:19 | jaypipes | dansmith: no. | |
| 01:52:35 | jaypipes | dansmith: I'll do it in the morning. will probably need to start from scratch with this last patch :( | |