| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-06 | |||
| 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 | |
| 21:16:47 | sean-k-mooney | http://paste.openstack.org/show/789247/ | |
| 21:16:55 | efried | nice | |
| 21:18:58 | sean-k-mooney | i will start trying different life cycle operation and testing and recording info on placement allcoation, db dumps and the like | |
| 21:19:44 | sean-k-mooney | i have a bash script i have written to automate this so ill script up a few test cases. and do some manually | |
| 21:20:34 | sean-k-mooney | since i have created a cyborg flavor i shoudl be able to run some of the standard tempest tests with that flavor | |
| 21:39:23 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix instance.hidden migration and querying https://review.opendev.org/706331 | |
| 21:39:30 | lifeless | stephenfin: hi, what do you need? mtreinish has commit rights on subunit | |
| 22:30:55 | efried | mnaser: did your +1 at PS3 here https://review.opendev.org/#/c/706331/ indicate that you had successfully tested this locally? | |
| 22:38:27 | mriedem | dansmith: i've got to run but will take a look at the latest later tonight | |
| 22:41:21 | dansmith | efried: I think he's going to have to apply it at the time he does his next upgrade which might not be for a while, it sounded like | |
| 22:44:33 | melwitt | dansmith: wouldn't your change unhide the instances for him today that are being incorrectly hidden? or are you saying he already fixed that via manual db update | |
| 22:44:58 | dansmith | melwitt: he already fixed up his db, as I understand it | |
| 22:45:05 | melwitt | gotcha | |
| 23:01:21 | sean-k-mooney | dansmith: i can check the code but are we not storing the resouce requests form the cybrog device profile in the request spec? | |
| 23:03:36 | sean-k-mooney | dansmith: im seeing "requested_resources": null in the request spec for the cyborg nova instance | |
| 23:04:01 | sean-k-mooney | the embeded flavor has "accel:device_profile": "FakeDeviceProfile" | |
| 23:05:12 | sean-k-mooney | however since we are not storing the groups if you change the device profil after the fact and we migration and instance or did something else that would need us to call plamcnet wwe would have to go back to cyborg which could have changed | |
| 23:18:24 | dansmith | sean-k-mooney: I think that's the idea | |
| 23:18:53 | dansmith | sean-k-mooney: you live migrate, scheduler calls placement with a new set of resources constructed from the device profile and what cyborg told you when you asked | |
| 23:19:34 | dansmith | sean-k-mooney: maybe we need to be doing something like examining the existing ARQs to generate those resource requests if the instance already exists? | |
| 23:19:56 | sean-k-mooney | new arqs sure but we dont want to hard reboot or live migate and change form an nvida gpu to an intel fpga | |
| 23:20:55 | sean-k-mooney | i think we need to be storing the groups retruned by cycborg when we instilly created the vm the same way we embed the flavor or image | |
| 23:20:57 | dansmith | not sure how that would happen on a hard reboot, but obviously agree on live-migration, but that's why I'm saying maybe we should look at the device profile on boot, and look at our existing arqs on any other move operation when asking cyborg for the resources | |
| 23:21:51 | sean-k-mooney | dansmith: i guess hardreboot it would not | |
| 23:22:03 | sean-k-mooney | we woudl just use the exising arq | |
| 23:22:10 | dansmith | maybe we need to ask sundar if the dp can change in a predictable or restricted way | |