| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-06 | |||
| 14:10:54 | mriedem | the fix is fine, but wondering about the 'this shouldn't happen' case | |
| 14:13:31 | dtantsur | will check in a few minutes | |
| 14:13:42 | dtantsur | yeah, I was also unsure whether to log or to raise | |
| 14:15:05 | mriedem | alex_xu: with the caching scheduler yes it was possible to create an instance with no allocations in placement, | |
| 14:15:14 | mriedem | because the caching scheduler didn't use placement or GET /allocatoin_candidates | |
| 14:15:17 | mriedem | the caching scheduler was removed in stein, | |
| 14:15:24 | mriedem | but you can still load up out of tree scheduler drivers that may not use placement | |
| 14:15:41 | mriedem | alex_xu: coincidentally, see https://review.opendev.org/#/c/680521/ | |
| 14:16:23 | cdent | mriedem: that mention of logging reminds me of something I wanted to ask about: In python 2, when using oslo_log and LOG.error it includes whatever the most recent exception was, even if it is out of scope. In pytyhon3 sys.exc_info() "expires" sooner, more predictably. This means that error messages in the scheduler report client often include bogus tails in the message | |
| 14:16:39 | cdent | do we care? or can we just ignore python2 soon enough? | |
| 14:18:03 | mriedem | cdent: i think i know what you're talking about, and melwitt/dhellmann had made some fixes to oslo.log to squash that weirdness, but maybe that was only for warning level logs, | |
| 14:18:29 | mriedem | e.g. you'd see a stacktrace for a ComputeNodeNotFound_Remote warning even though you weren't using exc_info=True on LOG.warning, | |
| 14:18:30 | cdent | it's warning and worse where oslo_log does the inspection exc_info | |
| 14:18:30 | mriedem | something like that | |
| 14:18:56 | melwitt | oh yeah, good memory | |
| 14:19:37 | melwitt | https://review.opendev.org/528036 | |
| 14:19:50 | mriedem | melwitt: i'm pretty sure task log is related to https://docs.openstack.org/api-ref/compute/#server-usage-audit-log-os-instance-usage-audit-log which is related to ceilometer which no one uses | |
| 14:19:57 | mriedem | i'd like to burn out all of that code | |
| 14:20:35 | melwitt | thanks. I was surprised in all my time working on nova, I've never heard of the task_log table before now | |
| 14:21:04 | melwitt | so I'd support burning the code that no one uses if indeed no one uses it | |
| 14:21:08 | efried | dtantsur: I looked briefly at the ironic CI job to see why it didn't hit that bug; I think it's just because those methods are used in the course of specific operations that tempest doesn't cover. We should add that, for sure. I'm not completely sure how to get started with that though. | |
| 14:21:30 | dtantsur | yeah | |
| 14:21:39 | dtantsur | the 2nd bug is hit when you have >1 nodes | |
| 14:21:47 | mriedem | rackspace probably had internal scripts or something to purge task log | |
| 14:22:02 | mriedem | melwitt: well clearly your customer is using it | |
| 14:22:09 | cdent | thanks melwitt, I think that corresponds to something I was looking at when poking at the problem, but doesn't address the issue with the exception name being included in the log even though it was "elsewhere". I'm inclined to ignore it if nobody is complaining, since it gets better with python3. I only noticed becasue somebody asked why there was a "NotImplemted" at the end of rp generation conflict message | |
| 14:22:46 | melwitt | mriedem: oh, you think table grow means they are using the audit APIs? I had assumed task_log entries get added regardless (like on any instance action) | |
| 14:22:49 | openstackgerrit | Dmitry Tantsur proposed openstack/nova master: Fix incorrect invocation of openstacksdk's baremetal.nodes() https://review.opendev.org/680684 | |
| 14:22:49 | dtantsur | mriedem: better ^^? | |
| 14:23:15 | alex_xu | mriedem: I'm talking to luyao, we should stop the instance booting if there is no allocation, since vgpu and vpmem should depend on allocation | |
| 14:23:22 | melwitt | I need to check | |
| 14:23:25 | mriedem | melwitt: it's a periodic task in the compute service which you have to enable, and it's disabled by default | |
| 14:23:32 | mriedem | so if they have task log table entries they are configured to run that task | |
| 14:23:34 | melwitt | oh, dang | |
| 14:23:42 | mriedem | https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.instance_usage_audit | |
| 14:23:56 | mriedem | and they are probably using ceilometer to hit that compute api | |
| 14:24:03 | mriedem | if they aren't, then they should disable the task and purge the db | |
| 14:24:03 | melwitt | thanks. I shouldn't have assumed | |
| 14:24:13 | mriedem | please see topic new contributor :P | |
| 14:24:24 | melwitt | :( | |
| 14:25:06 | mriedem | alex_xu: yeah that's probably fine, there is no real contract on support for out of tree scheduler drivers | |
| 14:25:34 | mriedem | alex_xu: see https://opendev.org/openstack/nova/src/branch/master/nova/scheduler/driver.py#L35 | |
| 14:27:41 | efried | dansmith: risk on https://review.opendev.org/#/c/677736/ I really have no idea I'm afraid. If cdent thinks it's okay to merge-and-watch, that's good enough for me. Remember FF is FF so we have a little bit of time before the release to flush out issues; so I don't think current proximity to FF is a reason to block it. | |
| 14:28:33 | dansmith | efried: ack | |
| 14:33:36 | mriedem | dansmith: before artom moves forward with this rpc/object change in the numa lm series https://review.opendev.org/#/c/634827/57//COMMIT_MSG@12 might want to see if you are on board | |
| 14:33:56 | mriedem | i'm basically asking that we're consistent in the flags saying both source and dest are ready for action | |
| 14:33:57 | alex_xu | mriedem: i see now, thanks | |
| 14:34:05 | dansmith | mriedem: ack looking | |
| 14:34:15 | artom | mriedem, actually yeah, thanks. I think it should be fine, but dansmith's lack of veto would boost confidence | |
| 14:35:16 | dansmith | artom: mriedem you're thinking set that field on the dest using the same can-do-numa thing in rpcapi? because without that, the only way to know is to pass a new param and have it *not* show up at the other end | |
| 14:35:34 | dansmith | we resolved that first, and then artom added the can-do check to rpcapi for the source, who has nobody to call | |
| 14:35:48 | dansmith | so if you're saying make the source and dest both call that, and set their corresponding field, yeah I guess that's okay | |
| 14:37:22 | artom | dansmith, not quite - I'm saying the dest doesn't need to actually send RPC 5.3, so check_can_live_migrate_source doesn't need that numa_live_migraiton flag, the dest can just shove it in migrate_data | |
| 14:38:17 | aspiers | Just asked in #openstack-qa but figured worth asking here too: Is it considered good/bad practice for tempest tests to check the message string of nova exceptions returned by assertRaises? | |
| 14:38:37 | dansmith | artom: but the dest needs to support the 5.3 inbound call, but you're saying actually report the conductor side? meaning the did-i-get-the-migration-from-conductor flag? | |
| 14:38:59 | aspiers | I'm thinking that if we wanted to reword an exception's message string, this could break tempest, and then you'd need a circular Depends-On across repos to fix it | |
| 14:39:10 | artom | dansmith, right, the dest needs to receive 5.3 - specifically migration and limits form the conductor | |
| 14:39:20 | artom | So if gets those, it can set the flag inside migrate_data | |
| 14:39:24 | dansmith | artom: and the new drop call | |
| 14:39:27 | dansmith | from source | |
| 14:39:31 | artom | dansmith, yeah | |
| 14:39:35 | artom | No, not from source | |
| 14:39:40 | artom | The source still needs to send 5.3 | |
| 14:39:43 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix incorrect invocation of openstacksdk's baremetal.nodes() https://review.opendev.org/680684 | |
| 14:39:51 | mriedem | dtantsur: +2 now ^ | |
| 14:39:52 | mriedem | efried: ^ | |
| 14:39:53 | dansmith | artom: the source calls the new drop-claim against the dest right? | |
| 14:40:02 | artom | dansmith, yeah | |
| 14:40:04 | artom | So if the dest sets the flag in migrate_data, these things could happen on the source: | |
| 14:40:09 | dansmith | artom: that's what I mean by inbound 5.3 | |
| 14:40:32 | dansmith | anyway, yeah I think that's okay | |
| 14:41:11 | mriedem | aspiers: error messages in the api aren't a contract | |
| 14:41:20 | mriedem | only error codes like placement uses, but nova doesn't | |
| 14:42:00 | mriedem | dansmith: yeah basically remove the numa_live_migration flag that goes from dest to source and instead put that as an attribute in migrate_data instead | |
| 14:42:01 | efried | dustinc: I'm merging https://review.opendev.org/#/c/680684/ but please have a look at it when you get in. | |
| 14:42:03 | mriedem | for consistency | |
| 14:42:04 | artom | dansmith, right, dest needs to receive 5.3 | |
| 14:42:12 | artom | But nothing needs to actually send it | |
| 14:42:15 | aspiers | mriedem: OK thanks, so I'll just check for HTTPBadRequest and leave it there | |
| 14:42:19 | efried | mriedem, dtantsur: +A | |
| 14:42:23 | artom | So if it sets the flag in migrate_data, we know it's the new code that can receive 5.3 | |
| 14:42:26 | artom | And then on the source: | |
| 14:42:37 | artom | Either it's old code and that flag gets dropped by the conductor | |
| 14:42:47 | dansmith | mriedem: right I was just confirming how we calculate that because right now the flag making it through the dest's rpcapi to the source is the sentinel | |
| 14:43:06 | artom | Either it's new code but pinned, in which case the can_send check will fail | |
| 14:43:15 | artom | Or it's new code unpinned, and we proceed with the NUMA LM | |
| 14:44:03 | mriedem | dansmith: yup | |
| 14:44:59 | mriedem | i didn't want to start a pattern of putting random feature flags on those compute rpc api methods, like sev=True, vpmems=True, suchandsuch=True | |
| 14:45:56 | dansmith | mriedem: yeah, but we needed it to figure out if the dest could *send* 5.3, but that's actually not a thing we need to know | |
| 14:46:11 | dansmith | and now we have that can-do method to tell us without actually trying to make the call | |
| 14:46:34 | artom | dansmith, that's the thing, I don't think dest actually needs to send 5.3 | |
| 14:46:42 | artom | The changes are all in objects | |
| 14:46:45 | artom | From its POV | |
| 14:48:07 | dansmith | artom: no it doesn't, I'm agreeing | |
| 14:48:31 | dansmith | artom: I think we just kinda munged the "does it support" with "is it configured to allow sending" ... 5.3 when we first discussed it | |
| 14:48:40 | artom | dansmith, yep | |
| 14:48:54 | dansmith | if we were adding new calls on both sides, they'd both need to be configured for 5.3, but in this case, we can cheat | |
| 14:50:56 | artom | dansmith, ack, looks like we're all on the same page, I'll continue down that route and with the rest of mriedem's feedback after the bug call | |
| 14:51:07 | dansmith | cool | |
| 14:52:26 | mriedem | stephenfin: alex_xu: yonglihe: on that topology cli change, this is wrong, correct? https://review.opendev.org/#/c/670790/7/doc/source/cli/nova.rst@3663 | |