| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-09 | |||
| 15:42:24 | bauzas | efried: see my comment on the spec | |
| 15:42:33 | bauzas | efried: about interop | |
| 15:43:23 | bauzas | efried: if people wanna add some query related to inventories, they should add it to Placement, not by a filter | |
| 15:43:33 | efried | bauzas: Responding. | |
| 15:43:48 | bauzas | efried: because the Placement logic needs to be interoperable for the same query | |
| 15:44:49 | mriedem | i just don't want to spend time making this change if nothing is going to leverage it in-tree right now | |
| 15:45:08 | bauzas | if we accept to pass back the inventories, then you'll be sure that one day or later, people will just use placement for basic checks, and will do python filtering for their specific calls | |
| 15:45:14 | mriedem | if out of tree filters/weighters want to get full provider tree and inventory information, they can query placement directly | |
| 15:45:17 | bauzas | and then, we're done with claims | |
| 15:45:26 | bauzas | what mriedem said | |
| 15:48:18 | jaypipes | what? | |
| 15:49:31 | efried | jaypipes: Help me out here. What's the reason we want to include non-requested resources and other-RPs-in-same-tree in provider summaries? | |
| 15:49:56 | edleafe | filters/weighers? | |
| 15:50:20 | jaypipes | efried: so that filters like the NUMATopologyFilter and ComputeCapabilitiesFilter can use the inventory usage and trait information. | |
| 15:50:33 | mriedem | if you need to filter on a resource class, that should be part of the requested resource in the query which puts it in the response, right? | |
| 15:51:02 | mriedem | this spec doesn't say anything about returning a full view of the resource provider traits either, only what was requested | |
| 15:51:11 | mriedem | or do you already get the full set of traits back in provider summaries? | |
| 15:51:12 | efried | mriedem: As I tried to explain, we have no syntax to talk about resource classes we're *not* requesting. | |
| 15:51:14 | jaypipes | mriedem: not if you want to know that a sibling NUMA node is "more packed" than another. | |
| 15:51:28 | efried | yeah, and that. | |
| 15:52:44 | mriedem | sure i get the weighing thing here | |
| 15:53:02 | mriedem | i'm basically -1 until something in-tree is going to leverage this | |
| 15:53:26 | mriedem | because provider 'summary' is not going to be a summary anymore, it's going to be a full blown view of the provider, it sounds like anyway | |
| 15:53:53 | efried | mriedem: That's really what was intended from the start, I believe. It was an artifact of the impl that it wound up being a subset. | |
| 15:54:21 | jaypipes | mriedem: we already return all the rp traits, not just the ones requested. https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/objects/resource_provider.py#L3426 | |
| 15:54:25 | efried | mriedem: Surely you're not suggesting that "something in-tree is going to leverage this" is necessary in the Rocky timeframe? | |
| 15:54:42 | mriedem | efried: why would that be shocking? | |
| 15:54:46 | jaypipes | mriedem: summary only referred to the aggregation of usage counts. | |
| 15:55:13 | efried | mriedem: Because especially with placement stuff, we've got a long history of implementing enablement before implementing the thing that exploits it. | |
| 15:55:27 | mriedem | efried: if a thing were queued up to exploit this, then it's a different story | |
| 15:55:58 | efried | mriedem: So you want a bp/spec for how NUMATopologyFilter is going to leverage this before you'll approve it? | |
| 15:56:18 | mriedem | depending on what that thing is, we might want to instead of just *always* returning the full inventory including the tree, maybe it's a parameter on the API to return something in addition to what we get today, based on config in nova which is based on which filters/weighters you're enabling | |
| 15:56:19 | efried | mriedem: Or is it enough to add more words to the use case paragraph? | |
| 15:56:59 | mriedem | efried: i just think this is getting the cart before the hose | |
| 15:57:01 | mriedem | *horse | |
| 15:57:23 | mriedem | and not something we should be spending time on given all of the other stuff that is actually planned to be used in this release | |
| 15:57:25 | jaypipes | mriedem: as I mentioned on the spec, I see this more of correcting an earlier mistake of not returning all resource class inventories/usage summaries in the provider_summaries section when we should have been doing that from the start. | |
| 15:57:50 | efried | ++ | |
| 15:58:06 | efried | Also, the code is already done | |
| 15:58:25 | mriedem | we never told anyone they should fully implement something before the bp is approved | |
| 15:58:35 | mriedem | code being done doesn't mean anything to me | |
| 15:58:51 | efried | from the perspective of spending time on it. | |
| 15:58:52 | mriedem | volume multiattach was 'done' in havana | |
| 15:59:18 | efried | Already has one core reviewer ready to +2, just holding up on spec approval. | |
| 16:00:00 | mriedem | and there is absolutely no unnecessary performance penalty on scheduling by returning full provider trees and inventory for all providers in a given request, even though we have nothing lined up to leverage it? | |
| 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? | |