Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-04
13:40:38 edleafe efried: It does seem that that's the best we can do here
13:40:44 edleafe Just wanted to get a second opinion
13:41:09 efried edleafe: So backing up for a second: today (before your code) if you try to create a consumer record with microversion <1.8, what happens?
13:41:39 efried ...because presumably the request payload doesn't require the proj/user IDs at <1.8
13:41:46 edleafe efried: the code checks for the presence of those two fields on the allocation object, and if they aren't there, or are None, it skips the creation
13:42:04 efried But how does it ultimately close the loop, then?
13:42:15 edleafe So my feeling is we should continue that behavior, even if it sucks
13:42:15 efried The allocation gets created with... no consumer?
13:42:25 edleafe efried: yep
13:42:49 efried So the consumer in that case is represented only by its UUID in the allocations table; it doesn't truly exist otherwise
13:42:55 edleafe TBC, no consumer *record*
13:43:10 edleafe it still has the instance UUID in the consumer_id field
13:43:27 efried ...of the allocations table
13:43:33 edleafe yes
13:44:46 efried so presumably if you GET an allocation record at 1.8 that was created at <1.8, the proj/user doesn't show up in the response.
13:44:54 efried or it shows up, but with null?
13:45:28 edleafe yeah, they are None >=1.8
13:45:42 efried Seems to me like the right thing would be to make proj/user be nullable.
13:45:55 efried and always create the record, even <1.8
13:46:09 efried ...which isn't a behavior change, because the API will still be doing exactly the same thing.
13:46:43 edleafe It's a teeny behavior change, but a real edge of an edge case
13:47:28 efried How would it be a behavior change? You're just changing the condition from "if the record doesn't exist, assume null/null" to "use what's in the record, which will always be there (oh, and btw, might be null)"
13:47:28 edleafe here's the case:
13:47:41 mriedem dansmith: i saw, but haven't dug into the changes yet
13:47:50 dansmith mriedem: okay just wanted to make sure you did
13:47:56 edleafe allocations are created <1.8. No consumer record in the past; now there is w/generation=0
13:48:15 efried dansmith, mriedem: You'll be delighted to know that I checked our OOT driver, and you don't break it with that change.
13:48:33 efried ...because we use the tuple form already.
13:48:58 efried edleafe: Sorry, still listening.
13:49:01 edleafe np
13:49:19 mriedem efried: i don't know why your out of tree driver would care about that change
13:49:52 edleafe A later attempt to allocate that provides the generation may or may not succeed, depending on the gen. If there were no consumer record, though, it would always succeed
13:50:02 edleafe IMO, that's not a bad change, but it is a change
13:51:27 openstack Launchpad bug 1761197 in OpenStack Compute (nova) "Not defined keypairs in instance_extra cellsV1 DBs" [Undecided,New] - Assigned to Surya Seetharaman (tssurya)
13:51:27 belmoreira dansmith mriedem can you have a look into https://bugs.launchpad.net/nova/+bug/1761197 and give me your opinion?
13:51:31 efried mriedem: https://github.com/openstack/nova-powervm/blob/master/nova_powervm/virt/powervm/tasks/network.py#L183-L186 where _get_vif_events uses the tuple form: https://github.com/openstack/nova-powervm/blob/master/nova_powervm/virt/powervm/tasks/network.py#L226
13:52:16 mriedem efried: that's how it's used in tree today too
13:52:18 efried edleafe: But the "later attempt to allocate that provides the generation" *should not* "always succeed"
13:52:33 efried mriedem: Right, I guess we've ported that code over already.
13:52:44 kashyap dansmith: Hi there, if you get time today, I too (like johnthetubaguy, who +2ed) am going blind on this: https://review.openstack.org/#/c/534384/
13:52:57 efried edleafe: Besides, there's not a behavior change wrt generation handling cause we don't *have* generation handling yet.
13:53:03 efried edleafe: IMO this is all tasty goodness.
13:53:10 edleafe efried: sure, but if we preserve the behavior with allocations <1.8 (who even uses that anymore?), it will always succeed
13:53:19 edleafe As I said, an edge of an edge case
13:53:20 mriedem efried: i guess you're referring to how event_names in that interface can be a list of names, rather than tuples https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L447
13:53:38 efried mriedem: right. Was allowable before dansmith's patch on your change.
13:54:07 dansmith belmoreira: hmm, are you saying that back when you ran the keypair migration they weren't sync'd down to the v1 cell databases and thus nothing happened?
13:55:26 dansmith efried: everyone copied the first implementation in libvirt I imagine, which was luckily done the more explicit way..I checked all the in-tree drivers
13:55:31 efried edleafe: Still not sure I follow. If you're using <1.8, you'll get generations incremented under the covers and you'll never know about it. As soon as you start using the new microversion (the one you're writing), you'll see the generation (whatever it is) when you GET, and you'll push it back when you PUT/POST. That's as it should be. But that behavior ought to be the same regardless of what we do with the proj/user
13:55:37 efried dansmith: nod
13:56:06 efried edleafe: The proj/user isn't required even at 1.8, is it?
13:56:33 efried hm, looks like the user_id isn't marked as optional.
13:56:59 efried sorry, neither is marked as optional.
13:57:36 efried but are they in fact required? (That would seem weird, tbh)
13:58:27 efried edleafe: But my point is that you would already have this weirdness if you straddled microversions like that. You would have to deal with the fact that proj/user were null before (whether it's because the record doesn't exist or because the record exists with nulls in it) and now are required (or whatever).
13:58:51 edleafe efried: they are required >=1.8
13:59:09 efried edleafe: We're probably talking in circles around an issue that's insignificant if it even exists at all.
13:59:23 efried Is there any reason we can't make those fields nullable?
13:59:43 purplerbot <edleafe> It's a teeny behavior change, but a real edge of an edge case [2018-04-04 13:46:43.828800] [n 2tEe]
13:59:43 edleafe efried: [t 2tEe]
14:00:02 edleafe Like, if you're a purist...
14:00:06 efried and make the logic that returns them tolerate <record absent> and <record present with nulls> with the same result.
14:00:52 edleafe sure, nulls would have to work in all cases
14:01:13 edleafe ok, let me make that change to the consumers table and see how that works
14:02:05 efried ++
14:08:57 openstackgerrit Surya Seetharaman proposed openstack/nova master: Cleanup RP and HM records while deleting a compute service. https://review.openstack.org/554920
14:13:56 openstackgerrit Merged openstack/nova master: Teardown networking when rolling back live migration even if shared disk https://review.openstack.org/555481
14:25:08 belmoreira dansmith: sorry was away... yes, the migration was only correctly done in top DB
14:25:46 dansmith belmoreira: but you mean the top api cell db, not the api_db, yes?
14:26:24 belmoreira yes, the top DB in cellsV1
14:26:35 dansmith belmoreira: we just removed the code that did that migration, but even still, it would be out of the ordinary to make nova-manage reach across cellsv1 DBs
14:28:48 belmoreira dansmith: yes I saw that the code was removed. But when running nova-manage in cells DBs it should go to nova_api for the keypair
14:29:29 belmoreira I can cook something to keep me going but not sure how many people will have the same problem when moving to cellsV2
14:29:39 dansmith oh..wait,
14:29:42 efried dansmith, mriedem: Any reason we should be waiting to merge https://review.openstack.org/#/c/558648/ ?
14:29:56 efried (and... what's Kevin's IRC nick?)
14:30:25 dansmith belmoreira: you're saying if you run the migration against a child cell it doesn't look to api_db for the keypair, but does if you run it against the api cell?
14:30:26 mriedem Vek
14:30:48 efried k, not on. Just wondering why he didn't +W it.
14:31:30 dansmith efried: *shrug* if it works I guess it's fine, I have no strong opinions
14:31:43 efried dansmith: ight, done.
14:32:12 mriedem efried: that patch doesn't cover fast8
14:32:15 mriedem which is the only thing i use
14:32:36 mriedem *only thing i use for pep8 locally
14:32:50 dansmith that's all I use too,
14:32:59 dansmith but it's run by a script, so maybe it's a different issue?
14:33:22 dansmith oh, I guess it's the same as pep8 actually
14:33:26 mriedem yeah
14:33:29 mriedem just smaller scope of changes
14:33:29 dansmith cdent: *on this
14:33:36 dansmith yeah
14:33:43 dansmith I thought it was different for some reason
14:33:49 belmoreira dansmith: no, what I was trying to say is that the original migration was not considering cells. It was only looking to the keypairs in the same DB. But, maybe we can have a new migration to look into the nova_api DB for the keypair when running nova-manage in child cells
14:33:51 mriedem efried: removing your -W doesn't pull it out of the queue
14:34:03 sean-k-mooney fast8 just runs pep8 on the files you changed in the current patch or working tree
14:34:09 efried mriedem: I suspected that might be the case :(
14:34:13 sean-k-mooney its a nice speed up
14:34:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Default to py3 for the pep8 tox env because it's stricter https://review.openstack.org/558648
14:34:23 mriedem that pulled it out
14:34:24 dansmith belmoreira: oh, did that one come before the ->api_db migration? it's not in my tree so I didn't go look

Earlier   Later