| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-26 | |||
| 17:53:01 | sean-k-mooney | dansmith: they dont use creat_all? | |
| 17:53:02 | dansmith | sean-k-mooney: do you mean only things that *test* the db migrations in specific ways? | |
| 17:53:06 | sean-k-mooney | that woul dbe much quicker | |
| 17:53:31 | sean-k-mooney | dansmith: i think its only set by thign that call migration.db_sync | |
| 17:53:43 | dansmith | sean-k-mooney: but that's a ton of things, right? | |
| 17:53:46 | dansmith | incidentally | |
| 17:54:05 | dansmith | things that want a DB setup have that done for them in the background right? | |
| 17:54:30 | sean-k-mooney | i dont know sound like i should add a function that will reset the gloabls and then either call that in test.py or in setup in the current test class | |
| 17:54:42 | dansmith | ++ for test.py for db things | |
| 17:54:57 | dansmith | DB stuff is so global and sticky IME that it belongs in the test.py, IMHO | |
| 17:55:03 | sean-k-mooney | dansmith: there are 2 ways to create the db. either start with blank schema or tell the model to create_all | |
| 17:55:18 | sean-k-mooney | ok ill do that so | |
| 17:55:36 | sean-k-mooney | still not conviced it the best way long term but ill do that for now | |
| 17:56:32 | melwitt | this is where we db_sync for every use of the Database fixture https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L643 | |
| 17:56:36 | dansmith | I feel like melwitt and I get extra argument points for the scars we have from all the cellsv1 and v2 test setup experience | |
| 17:56:38 | melwitt | and the CellDatabases fixture https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L397 | |
| 17:56:58 | melwitt | 😂 | |
| 17:57:20 | dansmith | by "argument points" i mean.. votes :) | |
| 17:57:55 | dansmith | I didn't fully read the backscroll, but melwitt you're pro-test.py right? | |
| 17:57:55 | sean-k-mooney | dansmith: well its more i have previosue used a different pattern for reseting global state mainly for cached stuff in the past | |
| 17:58:23 | sean-k-mooney | dansmith: that was nto the db cofnig but its the first time i have seen a prefernce to do this in test.py centrally | |
| 17:58:31 | melwitt | dansmith: yes | |
| 17:58:32 | sean-k-mooney | rahter then in the test module that cares | |
| 17:59:21 | sean-k-mooney | ok ill add a reset_globals function to the nova.db.migration module and then call that form test.py | |
| 17:59:26 | sean-k-mooney | ill push that in a few minutes | |
| #openstack-nova - 2021-08-27 | |||
| 01:31:15 | opendevreview | Qiu Fossen proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225 | |
| 01:32:55 | opendevreview | melanie witt proposed openstack/nova master: Revert "consumer gen: more tests for delete allocation cases" https://review.opendev.org/c/openstack/nova/+/806292 | |
| 01:32:56 | opendevreview | melanie witt proposed openstack/nova master: Revert "Consumer gen support for delete instance allocations" https://review.opendev.org/c/openstack/nova/+/806293 | |
| 01:36:51 | melwitt | gibi, sean-k-mooney, lyarwood: fyi I have uploaded reverts https://review.opendev.org/c/openstack/nova/+/806292 and https://review.opendev.org/c/openstack/nova/+/806293 as one possible option for addressing the gate bug. another option is the WIP patch https://review.opendev.org/c/openstack/nova/+/688802 to add a "force" kwarg to allocation delete | |
| 01:44:20 | opendevreview | Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294 | |
| 06:42:53 | slaweq | gibi: hi | |
| 06:43:09 | slaweq | gibi: I see a lot of job's failures with error on deleting instance, like e.g. https://a91ddba9ffca8a8b5d72-1d82fafbdd07c6a56856d7b1449f73a8.ssl.cf5.rackcdn.com/800059/3/check/neutron-ovn-tempest-ovs-release/fd05a18/testr_results.html | |
| 06:43:20 | slaweq | and it's not only on neutron-tempest-plugin jobs | |
| 06:43:30 | slaweq | I saw it also on some devstack jobs too | |
| 06:43:41 | slaweq | gibi: is it known issue or should I open one? | |
| 07:04:18 | gibi | slaweq: hi | |
| 07:04:26 | frickler | slaweq: I would be hoping that these are solved by the above reverts https://review.opendev.org/c/openstack/nova/+/806293 etc. | |
| 07:04:33 | gibi | just couple lines above melwitt proposed patches | |
| 07:05:35 | slaweq | gibi: frickler thx a lot | |
| 07:05:56 | gibi | I've just started my day but I will prioritize this issue | |
| 07:06:35 | slaweq | gibi: thx, I just wanted to ask as I knew that You will know better if that's something already reported :) | |
| 07:06:40 | gibi | sure | |
| 07:07:13 | frickler | slaweq: if you check n-cpu.log for errors, you'll see tracebacks about conflicts in placement allocations | |
| 07:09:07 | slaweq | frickler: yes, now I see it :) | |
| 07:37:04 | gibi | melwitt: thanks for proposing patches I read up on the bug discussion and the patches and I prefer the force kwarg that gives us a more fine grained approach to handle deletes in different situations | |
| 07:37:28 | gibi | I will resolve the merge conflict in https://review.opendev.org/c/openstack/nova/+/688802 and then I will +2 it | |
| 07:37:34 | gibi | sean-k-mooney, lyarwood, stephenfin: ^^ | |
| 08:19:55 | lyarwood | Morning | |
| 08:33:12 | gibi | lyarwood: morning | |
| 08:55:15 | lyarwood | gibi: sorry got distracted with $child things | |
| 08:55:35 | lyarwood | gibi: so how's the rebase going? Are we not going ahead with the reverts just yet? | |
| 08:55:48 | gibi | lyarwood: almost done | |
| 08:55:55 | lyarwood | ack | |
| 08:56:17 | gibi | lyarwood: for me the reverts are more complex than the force kwargs fix | |
| 08:56:26 | lyarwood | yup agreed | |
| 09:01:47 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802 | |
| 09:02:18 | gibi | lyarwood, sean-k-mooney, stephenfin: ^^ | |
| 09:15:30 | lyarwood | gibi: LGTM, had to do a little background reading to make sure but yeah still agree this is the better approach | |
| 09:19:10 | gibi | lyarwood: yepp the bug report has a good summary of what we are dealing with | |
| 09:28:06 | opendevreview | Fabian Wiesel proposed openstack/nova master: VmWare: Remove unused legacy_nodename regex https://review.opendev.org/c/openstack/nova/+/806336 | |
| 09:46:50 | gibi | lyarwood: thanks for the review on the pps series. I replied in https://review.opendev.org/c/openstack/nova/+/793619/15#message-a2916b6a95c6f4061429e9fb2898ed738dfc3427 | |
| 10:33:51 | opendevreview | FossenQiu proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225 | |
| 10:36:06 | opendevreview | Fabian Wiesel proposed openstack/nova master: Vmware: Fix spelling in test https://review.opendev.org/c/openstack/nova/+/806348 | |
| 10:43:43 | opendevreview | Fabian Wiesel proposed openstack/nova master: VmWare: Use of id shadows built-in function https://review.opendev.org/c/openstack/nova/+/806390 | |
| 10:45:41 | lyarwood | gibi: https://review.opendev.org/c/openstack/nova/+/688802 has some unit and functional failures if you have time to look | |
| 10:47:00 | opendevreview | Fabian Wiesel proposed openstack/nova master: Vmware: Fix indentation in conditionals https://review.opendev.org/c/openstack/nova/+/806391 | |
| 10:49:26 | gibi | lyarwood: whaat? I did run those tests before pushed | |
| 10:49:30 | gibi | looking | |
| 10:50:02 | lyarwood | yeah assumed you had, weird. | |
| 10:54:29 | gibi | obviously I run it on something else as I see failures that I havent seen before :/ | |
| 10:54:35 | gibi | anyhow fixing it ... | |
| 11:09:19 | gibi | the two failing functional test actually testing our bug that is now resulved with force=True | |
| 11:09:54 | sean-k-mooney | the reverts to me are still the better long term solution as over all i think it more complext to have 2 ways to delete things | |
| 11:10:40 | gibi | sean-k-mooney: but the automatic reclaim of soft deleted instance should not win over a user initiated restore of the same soft deleted instance | |
| 11:10:47 | sean-k-mooney | although the force flag patch is slightly smaller | |
| 11:12:18 | sean-k-mooney | hum... i can see that althoug i also dont think we should uspport soft deleted in general | |
| 11:12:47 | sean-k-mooney | so you are saying the added complexityu to support soft cdelete requires force | |
| 11:13:04 | sean-k-mooney | to ensure we can restore on the same host | |
| 11:13:22 | sean-k-mooney | without other allocations talking up the space | |
| 11:13:48 | sean-k-mooney | im still not sure we need to use a put in the soft delete case | |
| 11:14:00 | gibi | what I'm saying that a conflicting delete should be forced most of the time, except if the delete is conflicting with a restore of a soft deleted instance | |
| 11:14:49 | gibi | conflict means that restore receved first, we should not allow removing the allocation in this case blindly as it will lead to an active instance without allocaiton | |
| 11:15:16 | sean-k-mooney | would we not be able to do that with a lock on the instecak uuid | |
| 11:15:41 | sean-k-mooney | have restore aquire the lock and have the perodic try to aquire it too before it does the delete | |
| 11:16:28 | gibi | yes a lock would also be a solution | |
| 11:17:01 | sean-k-mooney | if we want to proceed with the force flag for now im not nessisarly against that | |
| 11:17:23 | sean-k-mooney | but im still not trilled by doing deletes with put in general | |
| 11:17:30 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802 | |
| 11:17:50 | gibi | sean-k-mooney: I'd like to proced with the force solution, reverting old patches feels messy. | |
| 11:18:04 | gibi | I need to go grab lunch, be back in 30 | |
| 11:19:16 | sean-k-mooney | ack enjoy | |
| 11:20:21 | sean-k-mooney | stephenfin: ya removing the globals is certenly another valid option. if there is no global state we have nothign to reset. ill take a look at that quickly | |
| 11:34:54 | opendevreview | sean mooney proposed openstack/nova master: db: Handle parameters in DB strings https://review.opendev.org/c/openstack/nova/+/805663 | |
| 11:34:54 | opendevreview | sean mooney proposed openstack/nova master: remove module level caching https://review.opendev.org/c/openstack/nova/+/806394 | |
| 11:35:42 | sean-k-mooney | stephenfin: ^ they were only ever used in that module so there was no test covergae for the gloabls so simple to remove them as you suggested | |
| 11:39:27 | gibi | sean-k-mooney: I agree less global is better | |
| 11:42:01 | sean-k-mooney | we can re add them if this really is a problem but personaly i would prefer to use the lru cache decorator on the function if it came to that | |
| 11:42:28 | sean-k-mooney | i tihnk stephen is correct that this likely is not providing much if any useful perfromance benifit today | |
| 11:44:14 | gibi | sean-k-mooney: lru cache decorator propbably store info on the func def, so that is also a global, just a hidden one. Still needs the same reset logic as a pure module level global | |
| 11:44:46 | gibi | I'm not sure what I like more, but I guess in this case a pure global is easier to notice | |