| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-16 | |||
| 17:06:52 | mriedem | dansmith: you want to check my rolling upgrade validation logic that i dumped in gibi's spec https://review.openstack.org/#/c/652608/4/specs/train/approved/server-move-operations-with-ports-having-resource-request.rst@190 ? | |
| 17:10:22 | dansmith | mriedem: I probably have to read the whole spec to make sense of that huh? | |
| 17:11:04 | imacdonn | efried, which one? i.e. do you think we need to address (2) or would fixing (1) obviate that ? | |
| 17:11:37 | efried | imacdonn: fixing (1) would obviate. But that assumes we can do so. | |
| 17:12:05 | imacdonn | efried, right ... so now I'm trying to understand why the row is being created in the first place | |
| 17:12:05 | efried | imacdonn: IMO we should fix (1) and change (2) to raise an explicit exception to "guarantee" it. | |
| 17:12:09 | efried | ++ | |
| 17:12:48 | efried | imacdonn: Is it possible for the rows to differ in any material way? | |
| 17:13:01 | efried | (i.e. a way that makes a difference to the outcome) | |
| 17:13:05 | mriedem | dansmith: not really, it's just the usual "how could this fail during an upgrade" | |
| 17:13:18 | mriedem | dansmith: he needs to pass new parameters to compute rpc api methods, | |
| 17:13:30 | dansmith | well, I read it and seemed like I needed to understand more, so I'm reading the wholething now | |
| 17:13:33 | mriedem | which could be (1) stein computes that don't handle those or (2) rpc pinned so we pop those parameters | |
| 17:13:36 | mriedem | ok | |
| 17:13:47 | imacdonn | efried, I'm assuming that _security_group_get_by_names() is used elsewhere (or at least intended to be reusable), so probably should consider all possible use-cases, if we tackle that one | |
| 17:14:45 | efried | used in two places | |
| 17:15:19 | mriedem | imacdonn: efried: honestly i'm not sure how much relevance that code even has anymore if you're using neutron | |
| 17:15:36 | imacdonn | mriedem, I was wondering about that | |
| 17:16:31 | mriedem | i think it at least means if you're using neutron, every project that ever created an instance in nova has a 'default' security_groups table record that is never used or cleaned up | |
| 17:16:33 | mriedem | vestigial | |
| 17:17:12 | imacdonn | that seems plausible | |
| 17:22:48 | dansmith | mriedem: see if my words help at all | |
| 17:26:47 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling cells v1 in '/os-hypervisors' API https://review.openstack.org/651292 | |
| 17:26:47 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove '/os-cells' REST APIs https://review.openstack.org/651291 | |
| 17:26:48 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'nova-manage cell' commands https://review.openstack.org/651294 | |
| 17:26:48 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling cells v1 in '/os-servers' API https://review.openstack.org/651293 | |
| 17:26:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove old-style cell v1 instance listing https://review.openstack.org/651296 | |
| 17:26:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling cells v1 for console authentication https://review.openstack.org/651295 | |
| 17:26:50 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'instance_fault_create_at_top' https://review.openstack.org/651298 | |
| 17:26:50 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'bdm_(update_or_create|destroy)_at_top' https://review.openstack.org/651297 | |
| 17:26:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'get_keypair_at_top' https://review.openstack.org/651300 | |
| 17:26:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'instance_info_cache_update_at_top' https://review.openstack.org/651299 | |
| 17:26:52 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'instance_update_from_api' https://review.openstack.org/651302 | |
| 17:26:52 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'instance_update_at_top', 'instance_destroy_at_top' https://review.openstack.org/651301 | |
| 17:26:53 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling cells v1 for instance naming https://review.openstack.org/651304 | |
| 17:26:53 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling 'update_cells' on 'BandwidthUsage.create' https://review.openstack.org/651303 | |
| 17:26:54 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop handling 'InstanceUnknownCell' exception https://review.openstack.org/651307 | |
| 17:26:54 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove cells code https://review.openstack.org/651306 | |
| 17:26:55 | openstackgerrit | Stephen Finucane proposed openstack/nova master: db: Remove cell APIs https://review.openstack.org/651309 | |
| 17:26:55 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove unnecessary wrapper https://review.openstack.org/651308 | |
| 17:27:08 | mriedem | dansmith: replied but yeah i think so | |
| 17:27:59 | dansmith | cool | |
| 17:28:42 | sean-k-mooney | mriedem: so related but sperate form gibis spec should we jsut use teh livemigration multiple port bindign workflow for all move operations | |
| 17:29:04 | sean-k-mooney | mriedem: i mentioned it in gibi's spec but it would simplfy things to have a singel common code path | |
| 17:29:53 | mriedem | given the issues NewBruce is hitting idk | |
| 17:30:36 | mriedem | moving all move operations to that model would be a big change i think, and likely not something we should block gibi's spec on | |
| 17:30:53 | sean-k-mooney | oh im not suggesting we should block on it | |
| 17:31:11 | sean-k-mooney | its jsut some/alot of the compleix would be reduced | |
| 17:31:37 | sean-k-mooney | but NewBruce issue is concering ill grant that | |
| 17:35:28 | francoisp_ | alex_xu, hi, would you have time to look at https://review.openstack.org/#/c/648123/6 - thanks! | |
| 18:05:11 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Do not create default security group during instance create if using Neutron https://review.openstack.org/653065 | |
| 18:05:12 | mriedem | imacdonn: let's see what blows up ^ | |
| 18:09:19 | imacdonn | mriedem: hmm .. wouldn't use_neutron be true in most cases, when the migration is being run ? | |
| 18:10:37 | mriedem | yes use_neutron is the default and what 99% of deployments are probably using at this point | |
| 18:11:10 | imacdonn | mriedem: I haz the dumb ... how would this solve the problem? | |
| 18:12:01 | mriedem | we don't hit the problem code if you're using neutron | |
| 18:12:17 | imacdonn | but .. I am using neutron, and I do hit the prpblem | |
| 18:12:25 | mriedem | with this patch | |
| 18:12:26 | mriedem | ? | |
| 18:12:48 | imacdonn | no, but the patch only makes a difference if you're not using neutron | |
| 18:12:51 | imacdonn | (?) | |
| 18:13:03 | mriedem | if not NEUTRON: create default sec group | |
| 18:13:14 | imacdonn | oh wait, I had it upside-down | |
| 18:14:35 | imacdonn | yeah OK ... I was about to propose making _security_group_ensure_default() return None if the context has no project_id (which does make the migration work for me) | |
| 18:16:31 | imacdonn | wonder if the migration hits https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L1740 | |
| 18:19:01 | imacdonn | mriedem: ^ I think it will | |
| 18:22:09 | imacdonn | mriedem: confirmed that I still hit the problem with your change, for above reason | |
| 18:24:50 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Do not create default security group during instance create if using Neutron https://review.openstack.org/653065 | |
| 18:24:52 | mriedem | try this ^ | |
| 18:28:18 | imacdonn | mriedem: that seems to work (at least allow the migration to work) | |
| 18:35:25 | mriedem | i still don't know how to recreate your issue | |
| 18:35:51 | imacdonn | you'd have to delete the marker instance (the one with uuid 00000000-0000-0000-0000-000000000000) | |
| 18:36:10 | mriedem | ok so like, | |
| 18:36:16 | mriedem | 1. run data migration, | |
| 18:36:26 | mriedem | 2. archive/purge deleted records | |
| 18:36:29 | mriedem | 3. run data migration | |
| 18:36:54 | imacdonn | I think you have to have at least one project with at least one instance | |
| 18:37:25 | imacdonn | (that's not deleted, I assume) | |
| 18:42:43 | aspiers | efried: haven't been following the channel but I'm around for the next few hours in case you get to reviewing the SEV spec | |
| 18:43:14 | efried | aspiers: ack | |
| 18:44:22 | imacdonn | mriedem: I'm not sure of the exact sequence that gets me into the bad state to begin with, but it has happened repeatedly (after both upgrade and fresh install) ... to force it, you may have to (delete marker instance, run the migration) twice | |
| 18:45:50 | mriedem | on a fresh install you wouldn't have any instances to migrate so i'm not sure how the marker is getting created | |
| 18:46:13 | mriedem | unless you mean: 1. create a test server, 2. run the migration, 3. delete the marker record 4. run the migration again | |
| 18:48:19 | imacdonn | mriedem: by fresh install, I mean that it was not an upgrade ... so like 1) install, 2) create a test instance ... some time later; 3) run the migration | |
| 18:48:39 | mriedem | yeah ok | |
| 18:54:21 | melwitt | random question: does a cold migration (no change in flavor) resize need to be resize confirmed like a flavor changing resize does? | |
| 18:55:26 | mriedem | yes | |
| 18:55:37 | imacdonn | Last time I tried, it was required... whether or not it *should* ...... | |
| 18:56:05 | melwitt | thanks y'all | |
| 19:09:35 | mriedem | imacdonn: well i'm unable to recreate your issue in a functional test but i found a new regression, 500 in the api | |
| 19:18:04 | cdent | mriedem: you're so good at that | |
| 19:20:13 | imacdonn | mriedem: hmm, I was just pondering if maybe the marker instance gets deleted when the last "real" instance for a project is deleted ... in testing that, I just got a "ClientException: Unknown Error (HTTP 504)" - not sure if related | |
| 19:20:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add regression test for bug 1825034 https://review.openstack.org/653098 | |
| 19:20:22 | mriedem | imacdonn: would be interested to know why my recreate steps for *your* bug don't hit here ^ | |
| 19:20:22 | openstack | bug 1825034 in OpenStack Compute (nova) "listing deleted servers from the API fails after running fill_virtual_interface_list online data migration" [High,Confirmed] https://launchpad.net/bugs/1825034 | |
| 19:21:27 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add regression test for bug 1825034 https://review.openstack.org/653098 | |
| 19:21:28 | openstack | bug 1825034 in OpenStack Compute (nova) "listing deleted servers from the API fails after running fill_virtual_interface_list online data migration" [High,Confirmed] https://launchpad.net/bugs/1825034 | |
| 19:21:56 | mriedem | imacdonn: the marker instance is soft deleted as soon as it's created | |
| 19:22:27 | mriedem | https://github.com/openstack/nova/blob/master/nova/objects/virtual_interface.py#L308 | |
| 19:23:25 | imacdonn | mriedem: ah, right, so it probably requires archival to have happened to reproduce my original problem | |