| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-11 | |||
| 15:59:37 | sean-k-mooney | mriedem: i think in general if the authour is not responcive that is enough to kick it | |
| 15:59:44 | sean-k-mooney | assuming it has -1 form a core | |
| 16:00:21 | sean-k-mooney | e.g. it need to be reworked but no active engagment form the owner | |
| 16:00:21 | tssurya | and I guess I have to bump microversion | |
| 16:15:29 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Convert nova-next to a zuul v3 job https://review.opendev.org/670196 | |
| 16:21:14 | mriedem | artom: pro tip, if rebasing https://review.opendev.org/#/c/667177/ is not necessary, don't do it | |
| 16:21:55 | sean-k-mooney | mriedem: gerrit need a diff diff setting | |
| 16:22:56 | sean-k-mooney | e.g. generate teh diff for the old patch against old base + new patch against new base and display the diff of those two diff per file | |
| 16:23:36 | sean-k-mooney | aka the eye ball diff we currently do but automated | |
| 16:24:31 | melwitt | yeah, been wanting that to exist for years. does git-review support something like that locally? I keep forgetting | |
| 16:25:31 | sean-k-mooney | not that i know of but it could | |
| 16:25:49 | sean-k-mooney | a 4 way diff is not that uncommon a thing | |
| 16:26:10 | Zara | (thanks for the suggestions so far btw; I disappeared to stare at the output of `nova list --debug` :) ) | |
| 16:27:43 | dansmith | git review does have a thing | |
| 16:31:41 | artom | mriedem, I know, not sure what happened | |
| 16:32:33 | artom | I guess I was careless | |
| 16:33:03 | artom | I must have `git rebase -i` to edit that middle patch, and forgotten that I'd updated the remote? | |
| 16:33:31 | mriedem | git review -R | |
| 16:33:56 | artom | Aha, noted | |
| 16:42:44 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Use SDK instead of ironicclient for node.list https://review.opendev.org/656027 | |
| 16:42:44 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Introduces SDK to IronicDriver and uses for node.get https://review.opendev.org/642899 | |
| 16:42:45 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Use SDK instead of ironicclient for setting instance id https://review.opendev.org/659690 | |
| 16:42:45 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Use SDK instead of ironicclient for validating instance and node https://review.opendev.org/656028 | |
| 16:42:46 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Use SDK instead of ironicclient for getting network metadata from node https://review.opendev.org/670213 | |
| 16:42:46 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Use SDK instead of ironicclient for add/remove instance info from node https://review.opendev.org/659691 | |
| 16:45:59 | efried | rebase = false | |
| 16:45:59 | efried | [gitreview] | |
| 16:45:59 | efried | mriedem: artom: or in .gitconfig, so you don't have to remember -R each time: | |
| 16:46:29 | melwitt | mriedem: do you know if the add/remove security group to/from instance API is a candidate for deprecation? was just looking at this abandoned patch after noticing the bug in launchpad https://review.opendev.org/535510 | |
| 16:46:56 | mriedem | efried: i don't personally want it in config since i make the decision each time | |
| 16:48:51 | mriedem | melwitt: i want to say it wasn't in https://specs.openstack.org/openstack/nova-specs/specs/newton/implemented/deprecate-api-proxies.html because it's an operation on the server which is not deprecated | |
| 16:48:54 | mriedem | even if it's a proxy to neutron | |
| 16:49:10 | mriedem | but we're not very consistent about that b/c the floating IP things in https://specs.openstack.org/openstack/nova-specs/specs/pike/implemented/deprecate-multinic-proxy-api.html are similar | |
| 16:49:49 | melwitt | mriedem: yeah, was thinking similar. at first I was comparing to attach interface but that's different bc you can't do that without nova. whereas it looks like you could do the add/remove secgroup to port without nova | |
| 16:49:51 | mriedem | addFloatingIP and removeFloatingIP would be in the same group as add/removeSecurityGroup on a server | |
| 16:51:23 | melwitt | ok, yeah. just wanted to ask bc the patch was abandoned on that basis (the potential for deprecation). otherwise, someone said they had tested the patch and it worked to fix the concurrent update problem | |
| 18:04:54 | mriedem | efried: i'm +2 on gibi's big heal_allocations change now https://review.opendev.org/#/c/637955/35 | |
| 18:05:03 | efried | oh goodie | |
| 18:05:13 | mriedem | i'm sure you can just rubber stamp it with no effort | |
| 18:05:27 | efried | I'm sure | |
| 18:05:37 | efried | been waiting for you to quit hammering at it before looking again | |
| 18:05:42 | efried | I'm sure it's nigh unrecognizable now. | |
| 18:05:49 | mriedem | not since you last looked on | |
| 18:05:50 | mriedem | *no | |
| 18:06:01 | mriedem | more tests, | |
| 18:06:09 | mriedem | and the rollback stuff was changed | |
| 18:06:33 | mriedem | he first tries to update ports before allocations and if the port updates fail or allocation update fails the port updates are rolled back | |
| 18:07:42 | efried | okay, all of that sounds familiar. | |
| 18:07:48 | efried | I'll hit it now. | |
| 18:18:21 | mriedem | i'm working on a fup patch for my nits | |
| 18:21:42 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Follow up for "nova-manage: heal port allocations" https://review.opendev.org/670361 | |
| 18:35:17 | openstackgerrit | Merged openstack/nova master: Add host and hypervisor_hostname flag to create server https://review.opendev.org/645520 | |
| 19:50:45 | efried | mriedem: gonna need your help understanding a thing here... | |
| 19:51:22 | efried | is this a patch-style update or a replace? | |
| 19:51:22 | efried | neutron.update_port(uuid, body=body) | |
| 19:51:53 | efried | i.e. does it only change the fields that are present in the body? | |
| 20:01:38 | mriedem | artom: sean-k-mooney: dansmith: https://review.opendev.org/#/c/667177/10 | |
| 20:01:50 | mriedem | efried: replace | |
| 20:02:00 | artom | zomgies | |
| 20:02:15 | mriedem | efried: are you worried about overwriting something in the binding profile during the rollback from the time we originally got the port? | |
| 20:02:47 | efried | mriedem: Actually I'm worried about blowing away the whole content of the port *except* the binding:profile['allocation'] when we're trying to set that guy. | |
| 20:03:04 | mriedem | efried: wait, | |
| 20:03:28 | mriedem | efried: link me to the line | |
| 20:03:36 | efried | mriedem: yes, sec. | |
| 20:03:41 | mriedem | but we do port binding updates like this in the neutronv2/api code as well | |
| 20:04:50 | efried | mriedem: https://review.opendev.org/#/c/637955/35/nova/cmd/manage.py@1896 and continuation https://review.opendev.org/#/c/637955/35/nova/cmd/manage.py@1926 | |
| 20:05:19 | melwitt | I didn't think it's like placement where anything not in the payload will get "erased" like a replace | |
| 20:05:55 | efried | well, there's a bug somewhere then. Because either we blow away everything but the allocation on the initial pass, or we do nothing on the rollback. | |
| 20:06:25 | efried | but mriedem yes indeed, I see port updates in neutronv2/api.py which are similar in spirit. | |
| 20:07:49 | melwitt | https://developer.openstack.org/api-ref/network/v2/?expanded=update-port-detail#update-port | |
| 20:08:17 | efried | melwitt: I read that; are you seeing something in there that implies "partial update"? | |
| 20:08:26 | efried | (I admit I didn't scrutinize it) | |
| 20:08:40 | mriedem | efried: agree that https://review.opendev.org/#/c/637955/35/nova/cmd/manage.py@1896 looks like trouble | |
| 20:08:50 | mriedem | https://developer.openstack.org/api-ref/network/v2/index.html?expanded=update-port-detail#revisions | |
| 20:08:56 | mriedem | neutron does have revisions and an If-Match header | |
| 20:09:03 | mriedem | like generations in placement and etags | |
| 20:09:18 | mriedem | anyway, that's more about what i thought you were going to bring up | |
| 20:10:10 | efried | I guess I would sort of hope you're not effing out of band with whatever ports you're trying to heal | |
| 20:10:53 | efried | but yeah, it would be nice to handle generations too. | |
| 20:11:03 | mriedem | looking at where this is used https://github.com/openstack/nova/blob/ff0feed25d56c8ccd2298d5b5b82e636880fa986/nova/network/neutronv2/api.py#L92 | |
| 20:11:12 | mriedem | it looks like we only ever modify the full binding:profile and then send back our updates | |
| 20:11:15 | mriedem | not just pieces of it | |
| 20:12:04 | efried | so like, update_port knows to replace the thing for the top-level key you're sending, but doesn't go deeper? | |
| 20:12:51 | mriedem | i would assume that the neutron API takes the value for whatever key in the PUT /ports/{port_id} request and replaces the value, not parts of the value | |
| 20:12:52 | efried | in which case the second one (rollback) is fine, but the first one is broken? | |
| 20:12:58 | mriedem | it would assume so | |
| 20:13:25 | efried | "whatever key in the PUT request" -- the key that's in there is 'port'. | |
| 20:13:35 | efried | you mean the one under 'port'? | |
| 20:13:38 | mriedem | yes | |
| 20:13:40 | efried | okay | |
| 20:13:42 | mriedem | keys in the port resource body | |
| 20:14:00 | efried | okay, so the first one is borked. I shall -1 accordingly. Thanks. | |
| 20:21:27 | cdent | all this fiddling suggests some kind of heal integration test would be a nice thing | |
| 20:21:38 | cdent | if one could be conceived | |
| 20:22:19 | efried | I think mriedem wrote one recently. Though I sort of doubt it went anywhere near this deep. | |
| 20:22:27 | mriedem | gibi has a patch for that eventually | |
| 20:22:34 | mriedem | fiddle me timbers | |
| 20:22:43 | efried | cdent: https://review.opendev.org/#/c/667994/ | |
| 20:22:50 | mriedem | https://review.opendev.org/#/c/669879/ | |
| 20:22:52 | mriedem | drats | |