| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-05 | |||
| 19:14:00 | mriedem | and if/when he replies, make sure it's related to powervm :) | |
| 19:15:09 | efried | mriedem Since you asked, I was working on filling in a gap we missed in the in-tree impl: the only thing we populate in the InstanceInfo is the state. | |
| 19:15:36 | efried | While looking, I found that docstring and recognized that we've never looked that guy up by name (even in the out-of-tree driver). | |
| 19:15:58 | efried | So I went to figure out whether there was any basis for that edict. | |
| 19:16:29 | mriedem | remove ", by name (not ID!)" from the docstring | |
| 19:16:38 | mriedem | it's always passed an Instance object | |
| 19:16:38 | efried | Doing it. Thanks. | |
| 19:19:47 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement project_id, user_id in PUT /allocations https://review.openstack.org/469634 | |
| 19:19:48 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: Add GET /usages to placement API https://review.openstack.org/469635 | |
| 19:19:48 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: use separate tables for projects/users https://review.openstack.org/470645 | |
| 19:20:04 | jaypipes | zzzeek: \o/ ^ | |
| 19:22:05 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Use util.extract_json in allocations handler https://review.openstack.org/471067 | |
| 19:24:20 | openstackgerrit | Eric Fried proposed openstack/nova master: ComputeDriver.get_info not limited to inst name https://review.openstack.org/471106 | |
| 19:24:31 | efried | mriedem ^^ This is why I earn the big bucks. | |
| 19:26:51 | mriedem | efried: it probably all ties back to the libvirt driver using this to get the guest info | |
| 19:26:51 | mriedem | return conn.lookupByName(instance.name) | |
| 19:27:03 | mriedem | most things in the virt driver layer are libvirt-biased | |
| 19:28:24 | efried | mriedem Don't I know it. | |
| 19:31:13 | efried | mriedem The other thing I noticed is that the only two consumes of get_info (in manager.py) only ever use the state. Makes me less panicky about hurrying to populate those other fields. | |
| 19:31:29 | efried | consumers* | |
| 19:47:27 | efried | jaypipes I presume the resource tracking stuff never plans to use get_info to figure out how much e.g. cpu/mem a VM is consuming - you're just going to record that at claim time and go with it? | |
| 19:50:05 | dansmith | efried: we don't know how much memory a guest is consuming without introspection | |
| 19:50:37 | dansmith | efried: and the amount of cpu it's using at some instantaneous point is not really an allocation thing | |
| 19:51:03 | efried | dansmith Okay, fair; but are there any plans for nova to actually use those InstanceInfo fields? | |
| 19:51:20 | dansmith | those...instanceinfo...fields... | |
| 19:51:23 | efried | Perhaps I'm missing some duck-typed dynamic usage of get_info | |
| 19:51:58 | dansmith | there are remnants of the extensible resource tracker and metric info in nova.. perhaps you mean that stuff? | |
| 19:52:18 | efried | dansmith https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L213-L217 <== those InstanceInfo fields | |
| 19:52:44 | efried | dansmith Perhaps I do mean that stuff. What stuff do I perhaps mean? | |
| 19:53:43 | dansmith | yeah, I _think_ that stuff is only used for choosing numa nodes | |
| 19:54:43 | dansmith | or maybe not even that, I don't even see that stuff considered in the scheduler | |
| 19:54:52 | efried | dansmith Does that happen in the nova tree? I'm even grepping and not finding any consumption of those fields. | |
| 19:55:55 | dansmith | efried: I think it's all within that file, and all for things like picking cpu topologies and stuff | |
| 19:56:05 | dansmith | but not a generic thing for reporting or choosing whole compute nodes, AFAIK | |
| 19:56:14 | dansmith | definitely not the material of placement though | |
| 19:56:45 | efried | dansmith What I'm considering doing here is leaving my InstanceInfo so it just provides the state. If anything ever gets written to consume any of the other fields, and assuming those code paths get hit in some tempest test, our CI will catch it (by raising AttributeError). | |
| 19:57:29 | efried | And at that time, we can figure out how best to implement those fields. Cause there's a number of options, with different performance characteristics, one of which would be more appropriate than another depending on the actual usage. | |
| 19:58:23 | dansmith | sounds reasonable | |
| 19:58:31 | mriedem | https://review.openstack.org/#/c/133777/ | |
| 19:58:42 | mriedem | looks like it was added for something...but very little context | |
| 19:58:52 | dansmith | yeah | |
| 19:59:28 | cfriesen_ | weird python question....how is the "return" value from main() in cmd/manage.py expected to be consumed? | |
| 19:59:48 | cfriesen_ | it doesn't seem to have any effect on the process return value if run from the shell | |
| 19:59:49 | efried | Recently, too - but only relative to the aforementioned "by name (not ID!)" thing ;-) | |
| 20:02:14 | mriedem | goes back to https://github.com/openstack/nova/commit/19da125805eedbfcfd202abac4a90c57e6c538c4 | |
| 20:02:49 | mriedem | https://github.com/openstack/nova/blob/c8e8b44ef27e49b3986659ee0cb6bd77b38430d8/nova/virt/libvirt_conn.py#L797 | |
| 20:02:59 | mriedem | so, it just always returned that other crap | |
| 20:03:05 | mriedem | because we could | |
| 20:03:15 | mriedem | because the libvirt domain api gave it to us | |
| 20:04:00 | mriedem | https://github.com/openstack/nova/blob/c8e8b44ef27e49b3986659ee0cb6bd77b38430d8/nova/virt/xenapi/vm_utils.py#L686 | |
| 20:05:51 | efried | Dig. I'll go with the plan, then. I'll just add a comment explaining it, so my posterity doesn't do a tim-the-tool-man like I did. | |
| 20:06:17 | mriedem | efried: there is one place where something besides 'state' is used, and it's in the libvirt driver's _destroy() method | |
| 20:06:23 | mriedem | checking to see if the domain id changed | |
| 20:07:23 | mriedem | unless there is another way to skin that id cat, we'd have to leave it, | |
| 20:07:33 | mriedem | but otherwise, if it were me, i'd remove all of that other unused garbage | |
| 20:07:36 | mriedem | from InstanceInfo | |
| 20:08:58 | efried | mriedem Okay, I'll look into it. | |
| 20:12:03 | efried | esberglu Can you answer cfriesen_ above? You figured out that the return values somehow mapped to exit codes? | |
| 20:15:40 | cfriesen_ | efried: esberglu: it appears that the return code is being mapped to the process exit value. it must be doing something clever. In any case, I opened https://bugs.launchpad.net/nova/+bug/1695965 because the return code for "nova-manage db online_data_migrations" isn't very helpful. | |
| 20:15:40 | openstack | Launchpad bug 1695965 in OpenStack Compute (nova) ""nova-manage db online_data_migrations" exit code is strange" [Undecided,New] | |
| 20:16:37 | efried | cfriesen_ Cool, esberglu just made a change on a similar theme: https://review.openstack.org/#/c/467218/ | |
| 20:16:56 | efried | cfriesen_ See bug https://bugs.launchpad.net/nova/+bug/1692982 | |
| 20:16:56 | openstack | Launchpad bug 1692982 in OpenStack Compute (nova) "Need --strict option for nova-manage cell_v2 discover_hosts" [Wishlist,Fix released] - Assigned to Eric Berglund (esberglu) | |
| 20:18:09 | cfriesen_ | efried: right. In the case of online_data_migrations we really want to distinguish between three separate scenarios, which doesn't map nicely to binary exit values | |
| 20:18:22 | efried | cfriesen_ In your case, it's possible the answer might be, "code to the API, not the CLI" ;-) | |
| 20:18:31 | efried | But I wouldn't be the one to advocate that. | |
| 20:19:32 | cfriesen_ | efried: heh...one nasty case is that the newton version of this command returns this "2 rows matched query pcidevice_online_data_migration, 0 migrated" even when there is no work left to do. | |
| 20:19:44 | cfriesen_ | outputs that, rather | |
| 20:20:01 | cfriesen_ | because the query is too generic. | |
| 20:21:37 | efried | cfriesen_ FWIW, dansmith was the one in our corner on that --strict thing. He had very useful feedback when the idea came up. | |
| 20:24:37 | cfriesen_ | dansmith: when you get a moment could you look at bug 1692982 ? | |
| 20:24:37 | openstack | bug 1692982 in OpenStack Compute (nova) "Need --strict option for nova-manage cell_v2 discover_hosts" [Wishlist,Fix released] https://launchpad.net/bugs/1692982 - Assigned to Eric Berglund (esberglu) | |
| 20:25:07 | cfriesen_ | dansmith: whoops, I meant bug 1695965 | |
| 20:25:07 | openstack | bug 1695965 in OpenStack Compute (nova) ""nova-manage db online_data_migrations" exit code is strange" [Undecided,New] https://launchpad.net/bugs/1695965 | |
| 20:25:47 | esberglu | efried: cfriesen_: Sorry was off for a few there. I think I dropped for some of the convo. You still need anything from me? | |
| 20:26:16 | cfriesen_ | esberglu: just wondering if you knew offhand how the return value of "nova-manage" gets converted to process exit value | |
| 20:26:17 | dansmith | cfriesen_: we had migrations in the past that raised an exception when something wasn't new enough, but you needed to proceed when in order to get to the newer stuff, hence the zero return in that case | |
| 20:26:37 | dansmith | thats why it was like that | |
| 20:26:43 | dansmith | but I don't disagree with nailing it down as you describe | |
| 20:27:22 | cfriesen_ | dansmith: also, in Newton you can't even tell from the output logs whether or not you need to call it again, because of the PCI issue I showed above | |
| 20:27:44 | cfriesen_ | dansmith: the "count_all" is counting cases where there is no migration to be done | |
| 20:28:11 | esberglu | efried: Not sure. I just took if granted based on other commands | |
| 20:47:41 | cfriesen_ | dansmith: related issue: https://bugs.launchpad.net/nova/+bug/1695991 | |
| 20:47:41 | openstack | Launchpad bug 1695991 in OpenStack Compute (nova) ""nova-manage db online_data_migrations" doesn't report matched/migrated properly " [Undecided,New] | |
| 20:54:04 | mriedem | dansmith: i think we've touched on this before, but since services are restricted to cell dbs, we don't have a global view of what a minimum compute version is in a deployment once we have multpile cells | |
| 20:54:06 | mriedem | thinking of https://github.com/openstack/nova/blob/78c69f61ab186c3d3eb0fe5cc5114cc71f0fbdad/nova/objects/service.py#L425 | |
| 20:54:36 | dansmith | mriedem: I have a helper | |
| 20:54:40 | mriedem | if we have an instance we can find the cell it's in and get the minimum compute for that cell, or get the service version for the compute the instance is on | |
| 20:55:59 | cfriesen_ | how is Service._check_minimum_version() going to work with cells then? check just the current cell? | |
| 20:56:16 | mriedem | well we could certainly iterate cells, | |
| 20:56:22 | mriedem | find the min for $service in all cells, | |
| 20:56:27 | mriedem | and return the lowest | |
| 20:56:42 | dansmith | mriedem: I'm sure I already wrote that helper.. looking | |
| 20:57:06 | mriedem | i randomly think about this because we don't really test for it in our grenade tests | |
| 20:57:12 | mriedem | since grenade is so limited in scope to smoke tests | |
| 20:57:59 | mriedem | e.g. https://github.com/openstack/nova/blob/78c69f61ab186c3d3eb0fe5cc5114cc71f0fbdad/nova/api/openstack/compute/servers.py#L496-L499 | |
| 20:58:06 | mriedem | ^ won't be accurate with multiple cells | |
| 20:58:24 | mriedem | i'll open a bug to track this | |
| 20:58:37 | dansmith | and unless we had multiple real cells we wouldn't hit it anyway | |
| 20:58:53 | mriedem | we had == in ci? | |