Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-06
19:06:28 dansmith I'm on py37
19:07:53 efried dansmith: zuul agrees with me
19:09:40 efried but it also fails despite the migration having been 'fixed'.
19:10:39 dansmith hrm
19:10:55 dansmith what version of sqla is in your venv?
19:11:23 efried I just repro'd with a fresh py37 env, so whatever's in requirements.
19:11:31 dansmith can you look so I can compare?
19:11:45 efried SQLAlchemy==1.3.13,sqlalchemy-migrate==0.13.0,sqlparse==0.3.0
19:11:51 sean-k-mooney tox -e py37 -- nova.tests.unit.db.test_migrations.TestNovaMigrationsSQLite.test_walk_versions
19:11:59 sean-k-mooney passes form me with dans change
19:12:01 dansmith okay I just did a fuuuul run and that failed like yours but doesn't fail for me on just the module
19:12:12 dansmith so must be an ordering thing or something?
19:12:16 efried or something.
19:12:19 mriedem shouldn't column_default_banned default to False for everything before 399?
19:12:31 efried I skimmed looking for other monkey patches of Column, but didn't see any at a glance.
19:12:35 mriedem i guess the default maybe doesn't matter since the test is always passing a value
19:12:39 dansmith mriedem: it does.. set it to zero and it will fail for 216_havana
19:13:29 mriedem yeah it was the default in the fixture that i was thinking about - the default isn't used since the test always overwrites it
19:13:44 dansmith correct
19:13:54 efried if I'm being super picky, either remove the default or specify the kwarg name in the call.
19:14:14 efried but that's so nittish for a test I won't even put it in the review
19:14:14 dansmith efried: you see that the original doesn't specify either I assume
19:14:24 dansmith the existing argument I mean
19:14:35 efried hah, the number of times we've busted Sundar for following a precedent that was wrong...
19:14:43 dansmith python2 used to not allow multi-line with statements so it had to be short
19:14:46 mriedem only other comment on the actual fix is do you want a reno to advertise it since it's going to be backported
19:15:03 dansmith mriedem: yep
19:15:12 mriedem can i -1 for it to get stats!?
19:15:44 mriedem after that i'd be +2 on the fix
19:16:16 mriedem well i guess there is no test for the quota thing but i'll leave that up to you guys,
19:16:38 mriedem in my new job tests are a low concern for people so i'm getting used to not asking for them.
19:16:41 sean-k-mooney efried: dansmith ya i also can repoduce the error with "tox -e py37 -- nova.tests.unit.db.test_migration" so this is like due to incorrect mocking
19:16:46 dansmith mriedem: I didn't because we kinda already test the non-null site, and didn't want to have to replicate the raw-sql create of a null-having record for that too, but I can
19:17:01 mriedem up to you, like i said, any tests are good :)
19:19:52 mriedem mnaser: now that you're getting to train does this mean you're going to start cross-cell resizing like a mad man?
19:20:15 mriedem oh wait, did that land in ussuri?
19:20:35 mriedem ah right it's available in ussuri, nvm me
19:21:13 dansmith mriedem: he would need multiple cells to cross :)
19:21:24 mriedem i know, it was implied as a nudge
19:21:34 mriedem because there has been some vexxhost multi-cell chatter for awhile
19:21:49 mriedem does cern still upgrade nova?
19:37:18 dansmith ah I see the problem
19:37:37 dansmith I dunno why it doesn't always happen, but it's also not going to be an easy fix :/
19:39:49 mnaser mriedem: hah. yeah,ii think for cross-cell there's a few mountains to climb first
19:40:09 mnaser like figuring out glance with multiple backends and nova cells with different ceph backends in each one
19:40:24 mnaser and time
19:40:46 dansmith mnaser: that's in the works fwiw
19:40:58 dansmith glance has to do a thing first and then I plan to get on the nova side
19:46:41 dansmith efried: mriedem: the reno for this should just have an item in "fixes:" ? or critical? or upgrade?
19:47:34 mriedem either fixes or upgrade, or both i guess?
19:48:27 mriedem so i guess if you do upgrade, you can say if you haven't rolled to this point yet make sure you do first rather than like train GA, but if you have already upgraded to train GA and hit this issue, you can manually update the records (maybe after an archive/purge)?
19:48:39 mriedem my guess is people that hit this will be looking for some guidance on what to do
19:52:42 dansmith aight
20:03:44 melwitt does anyone know if there's a similar issue with a column default when using alembic for migrations?
20:04:11 openstackgerrit Sundar Nadathur proposed openstack/nova master: Add Cyborg device profile groups to request spec. https://review.opendev.org/631243
20:04:11 openstackgerrit Sundar Nadathur proposed openstack/nova master: ksa auth conf and client for Cyborg access https://review.opendev.org/631242
20:04:12 openstackgerrit Sundar Nadathur proposed openstack/nova master: Create and bind Cyborg ARQs. https://review.opendev.org/631244
20:04:12 openstackgerrit Sundar Nadathur proposed openstack/nova master: Define Cyborg ARQ binding notification event. https://review.opendev.org/692707
20:04:13 openstackgerrit Sundar Nadathur proposed openstack/nova master: Compose accelerator PCI devices into domain XML in libvirt driver. https://review.opendev.org/631245
20:04:13 openstackgerrit Sundar Nadathur proposed openstack/nova master: Pass accelerator requests to each virt driver from compute manager. https://review.opendev.org/698581
20:04:14 openstackgerrit Sundar Nadathur proposed openstack/nova master: Enable hard/soft reboot with accelerators. https://review.opendev.org/697940
20:04:14 openstackgerrit Sundar Nadathur proposed openstack/nova master: Delete ARQs for an instance when the instance is deleted. https://review.opendev.org/673735
20:04:15 openstackgerrit Sundar Nadathur proposed openstack/nova master: Enable and use COMPUTE_ACCELERATORS trait. https://review.opendev.org/699554
20:04:15 openstackgerrit Sundar Nadathur proposed openstack/nova master: Enable start/stop of instances with accelerators. https://review.opendev.org/699553
20:04:16 openstackgerrit Sundar Nadathur proposed openstack/nova master: Add cyborg tempest job. https://review.opendev.org/670999
20:04:16 openstackgerrit Sundar Nadathur proposed openstack/nova master: Bump compute rpcapi version and reduce Cyborg calls. https://review.opendev.org/704227
20:20:19 mriedem melwitt: i'm not actually sure if it's sqlalchemy or sqlalchemy-migrate that is applying that default value to existing records,
20:20:23 mriedem probably need to ask zzzeek
20:21:02 dansmith there are two things here: first reading the null values and needing to do the defaulting is a SQLA thing, not related to alembic
20:21:17 dansmith second is the application of the default to the existing rows, which could be different
20:21:27 melwitt asking because the proposed consumer_types table in placement is specifying a default column value https://review.opendev.org/#/c/669170/10/placement/db/sqlalchemy/alembic/versions/422ece571366_add_consumer_types_table.py@83
20:21:46 melwitt ok
20:21:56 dansmith melwitt: should be pretty easy to test.. I'm sure you have a devstack with that applied for your own testing
20:22:20 dansmith melwitt: just create rows without the patch applied, then apply and roll over that migration and see if the field for existing rows is NULL or the default
20:22:28 melwitt yes ... right ...
20:25:34 mriedem bingo bango https://opendev.org/x/sqlalchemy-migrate/src/branch/master/migrate/changeset/schema.py#L594
20:25:43 mriedem dansmith: there is the proof
20:26:02 mriedem so you *could* use default= in schema migrations but you'd have to also set populate_default=False
20:26:12 dansmith yeah, but no
20:26:26 dansmith not sure what purpose that would serve
20:26:38 dansmith if we shared Column() defs with migrations and models or something maybe
20:26:48 dansmith but the models in sync test didn't even fail for me
20:31:53 melwitt I'm realizing there's a difference between 'default' and 'server_default'? the placement table is using 'server_default'
20:35:18 melwitt https://stackoverflow.com/questions/14002631/why-isnt-sqlalchemy-default-column-value-available-before-object-is-committed#14013090
20:35:20 zzzeek melwitt / mriedem not reading everyhing but when you add a column to a database that has a default value and it's "not null", the DB adds that default. that is how you get a MySQL migration that is very slow for large tables btw
20:38:02 melwitt zzzeek: is that true regardless of whether it's specified as a 'default' vs a 'server_default'? will 'server_default' also try to backfill in already existing records that do not have a value set?
20:38:16 zzzeek melwitt: oh...server default only, sorrhy
20:38:33 zzzeek melwitt: for "default" that is not a server default, I have no idea what sqlalhcemyt-migrate does
20:38:53 zzzeek i'd be surprised if they use it, though, because the "add not null column / populate server default" is necessarily atomic
20:39:00 zzzeek you can't do that from the client using a python-side default
20:39:12 melwitt zzzeek: ok, it also does a backfill, this is the patch where it's being fixed if you're curious https://review.opendev.org/706331
20:39:23 zzzeek only if migrate takes the crazy insane step of making the column as nullable first, then populating, then not-nulling
20:39:43 melwitt zzzeek: this is the link from migrate https://opendev.org/x/sqlalchemy-migrate/src/branch/master/migrate/changeset/schema.py#L594
20:40:12 zzzeek melwitt: wow, yuck :)
20:40:17 melwitt lol
20:40:20 zzzeek i hate migrate
20:41:04 zzzeek b.c. you know that fails if the DB is live and new rows still getting added
20:41:26 melwitt mnaser knows ;)
20:47:04 mnaser yeah, i do
21:15:51 sean-k-mooney Sundar: i redeploy and have been able to boot a vm with the fake cyborg driver

Earlier   Later