Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-09
16:00:10 mriedem even if i get back 1000 of these things now?
16:01:40 efried For the back-end retrieval, no. As far as wire traffic, in the common case we have today, there will be *almost* no difference in the payloads anyway.
16:02:13 efried cfriesen_: You feel like writing something up about how NUMATopologyFilter will exploit provider_summary data?
16:06:07 edleafe Can anyone with more sqla-fu explain why the migration [0] and the model here [1] are not in sync?
16:06:10 edleafe [0] https://review.openstack.org/#/c/557958/4/nova/db/sqlalchemy/api_migrations/migrate_repo/versions/059_add_consumer_generation.py
16:06:13 edleafe [1] https://review.openstack.org/#/c/557958/4/nova/db/sqlalchemy/api_models.py
16:06:32 efried jaypipes: ^ I couldn't tell at a glance either edleafe
16:07:08 openstackgerrit Eric Berglund proposed openstack/nova master: PowerVM: Add proc_units_factor conf option https://review.openstack.org/554688
16:11:01 jaypipes edleafe: maybe try removing all of the server_default stuff.
16:11:07 efried esberglu: As takashin mentioned, ^ needs a reno
16:11:22 esberglu efried: Yep, posted a comment saying the same
16:11:26 jaypipes edleafe: perhaps it's a thing where the difference between sqlalchemy-migrate and sqlalchemy's models are weird.
16:11:38 efried esberglu: Oh, see it now.
16:11:39 esberglu Didn't see it before
16:21:14 edleafe jaypipes: I'll give it a try
16:22:45 jaypipes edleafe: step 2: yell at jaypipes about even mentioning server_default. ;)
16:25:06 edleafe jaypipes: only if the test passes now :)
16:25:29 jaypipes hehe
16:27:29 edleafe jaypipes: you're lucky. The test failed
16:27:46 jaypipes hehe
16:27:50 edleafe jaypipes: sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) Cannot add a NOT NULL column with default value NULL [SQL: u'\nALTER TABLE consumers ADD generation INTEGER NOT NULL'] (Background on this error at: http://sqlalche.me/e/e3q8)
16:28:11 jaypipes ahhh
16:29:26 jaypipes edleafe: this is ringing a bell... I think dansmith ran into a similar issue in the past.
16:30:11 jaypipes edleafe: I think it's a sqlite-ism
16:30:15 jaypipes edleafe: one sec
16:30:18 dansmith doesn't ring for me
16:30:44 cdent mriedem: I added a couple of placement-related things to http://forumtopics.openstack.org/
16:30:52 cdent just sos you're aware
16:31:36 jaypipes edleafe: sqlite doesn't support full alter table.
16:31:59 jaypipes edleafe: so you will need to do a funky script for sqlite :(
16:32:10 edleafe orly?
16:32:18 jaypipes edleafe: I can try to give it a shot if you'd like, but wouldn't be able to get to it until tomorrow likely
16:32:24 jaypipes edleafe: yeah, lemme grab an example
16:32:29 edleafe ok, let me do some googling
16:35:05 edleafe so it's not the server_default stuff; it's changing the nullable for the project_id and user_id fields that sqlite doesn't support
16:37:59 jaypipes edleafe: no
16:38:20 jaypipes edleafe: it's the addition of the generation column to the consumers table as a NOT NULL field that fails.
16:38:42 jaypipes edleafe: because that create_column() ends up being translated to an ALTER TABLE consumers ADD COLUMN generation NOT NULL
16:39:00 jaypipes edleafe: and SQLite's ALTER TABLE ... ADD COLUMN doesn't support a DEFAULT clause :(
16:39:49 jaypipes edleafe: so you need to do a CREATE TABLE consumers_new with the new schema, then do a SELECT into the new table, then do a DROP TABLE consumers; RENAME TABLE consumers_new TO consumers
16:39:56 jaypipes edleafe: like I said, it's a pain in the ass.
16:40:16 jaypipes edleafe: and I'm wondering if we need to do it for SQLite anyway since it's dev-only DB.
16:40:19 edleafe from the sqlite docs: "Only the RENAME TABLE and ADD COLUMN variants of the ALTER TABLE command are supported. Other kinds of ALTER TABLE operations such as DROP COLUMN, ALTER COLUMN, ADD CONSTRAINT, and so forth are omitted."
16:40:40 jaypipes edleafe: yes, but the DEFAULT clause of ADD COLUMN isn't supported, IIRC
16:41:20 edleafe jaypipes: sure, but neither would the ALTER COLUMN calls for those other two fields, right?
16:41:24 jaypipes so you can't do: ALTER TABLE consumers ADD COLUMN generation INTEGER NOT NULL DEFAULT 0
16:41:48 openstackgerrit Chris Dent proposed openstack/nova master: Use nova.db.api directly https://review.openstack.org/543262
16:41:57 jaypipes edleafe: yes, likely. it's probably that those errors haven't (yet) come up due to the failure of the consumers.generation thing first.
16:42:03 edleafe jaypipes: ok, I'll give it a shot. It would be *so* much easier in SQL than sqla.
16:42:38 edleafe jaypipes: although I did lift the nullable change from nova/db/sqlalchemy/migrate_repo/versions/267_instance_uuid_non_nullable.py
16:42:54 jaypipes edleafe: so you'll need to add a file in the migrate_repo called 059_add_consumer_generation.sql I believe.
16:43:25 edleafe and that made it in
16:43:38 jaypipes edleafe: and then guard the 059_add_consumer_generation.py file with the whole dialect != 'sqlite' thing
16:44:54 jaypipes edleafe: alternately, you could just execute raw sql statements like is done here: https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api_migrations/migrate_repo/versions/044_placement_add_projects_users.py#L63-L77
16:44:56 edleafe jaypipes: so the .sql file will be executed automatically? Or do I call it from the .py?
16:45:13 jaypipes edleafe: pretty sure it would get executed automatically, but lemme check
16:45:19 edleafe ah, I like that better
16:48:37 jaypipes edleafe: so apparently ALTER TABLE ... ADD COLUMN in SQLite *does* support the DEFAULT clause.
16:48:47 jaypipes edleafe: so I'm not sure why it's not being generated here.
16:57:19 jaypipes edleafe: ok, I'm a bit stumped... may need to reach out to zzzeek_ on this one. can you try setting default="0" instead of default=0 in both the migration and models?
16:58:22 jaypipes edleafe: from the error message it kinda seems like the difference that oslo.db's test_models_in_sync thing is seeing has to do with '0' vs. a sqlalchemy.sql.elements.TextClause objectTextClause
16:58:43 openstackgerrit Surya Seetharaman proposed openstack/nova master: Cleanup RP and HM records while deleting a compute service. https://review.openstack.org/554920
17:02:10 dansmith mriedem: I think we already have a place where we need placement configured for nova-api
17:02:18 dansmith although I think we're not actually doing the thing that would require it
17:02:27 dansmith which is local delete nuking allocations
17:04:39 edleafe jaypipes: no dice on default="0"
17:04:43 edleafe jaypipes: same error
17:05:34 jaypipes :(
17:12:31 mriedem dansmith: yeah that's a latent bug, which i've had sitting in my todo list,
17:12:36 mriedem i'll tackle it at 3pm on friday :)
17:12:58 dansmith that's fine, just don't expect me to be around for it :)
17:13:14 mriedem you said you'd always be there for me
17:14:16 mriedem avolkov: some easy changes to make in https://review.openstack.org/#/c/511183/ - just got feedback from dtroyer too
17:14:35 mriedem although i know it's late in the day for you
17:23:21 mriedem dansmith: when you get a sec, there is a mini debate in my patch to wait for vif plugged events before starting live migration that could use your input https://review.openstack.org/#/c/558001/ - some of it around whether or not we should even fail if we timeout, which thinking back on sahid's recent patch for linuxbridge, that doesn't fail on timout, it just logs a warning and continues
17:23:34 openstackgerrit Merged openstack/nova-specs master: mirror nova host aggregates to placement API https://review.openstack.org/545057
17:23:51 dansmith mriedem: no, I made him fail if it times out
17:24:09 mriedem yeah i see that now https://github.com/openstack/nova/blob/e2d5dc4e2c5f69fcbcb04c29b2ed469725122787/nova/virt/libvirt/driver.py#L7429
17:24:21 mriedem so it's totally unconditional based on the CONF.vif_plugging_is_fatal option
17:24:58 dansmith if we're expecting an event and don't get it, we should fail just about anything, IMHO, except if we're waiting during delete or something
17:25:08 mriedem that was basically my reply in my patch
17:25:13 mriedem if i set this, i expect an event
17:25:26 mriedem fail if it doesn't happen
17:25:55 dansmith totes
17:33:30 mriedem dansmith: on https://review.openstack.org/#/c/558059/4/nova/compute/manager.py@368 did you want to update that and add yourself as co-author?
17:33:34 mriedem otherwise the change lgtm
17:33:54 mriedem ill leave it up to efried and jaypipes to love up on it otherwise
17:35:34 dansmith mriedem: I don't care, but if you want blame-sharing I'll be glad to
17:35:47 mriedem updated that comment, event.key is probably ok
17:36:00 mriedem so just drop the wip if you don't want co-blame
17:36:20 dansmith done
17:41:39 efried mriedem, dansmith: +2. Who's gonna be the second +2?
17:42:00 mriedem i only mentioned two people to love up on it
17:50:08 zzzeek_ jaypipes: server_default in SQLAlchemy for a DEFAULT clause
17:50:36 dansmith ah, now that does sound familiar
18:09:22 edleafe zzzeek_: Hey, can you take a look at a migration issue I'm having?
18:09:47 jaypipes zzzeek_: we've tried adding and removing server_default with no luck...
18:10:35 edleafe jaypipes: I tried the recreate, import, drop, rename dance, but it seems to have a problem with the autoincrement
18:11:01 mriedem dansmith: just need a simple unit test add to https://review.openstack.org/#/c/539590/ and i'm +2
18:11:14 mriedem i know that's an older series

Earlier   Later