| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-09 | |||
| 14:33:28 | dansmith | then you aren't breaking a node that is active with no RC since we're not doing anything with those yet | |
| 14:33:29 | dansmith | however, | |
| 14:33:36 | dansmith | I think it's far more confusing to allow that | |
| 14:33:43 | dansmith | not worth the confusion over consistency | |
| 14:34:13 | mriedem | jaypipes: dansmith: i'm inclined to -1 this for missing tests https://review.openstack.org/#/c/491850/ but given dansmith is out the next two days i realize there is a need to start pushing this code through | |
| 14:34:15 | dtantsur | this is what I'm thinking about. a user creates an instance back in Ocata, no resource_class set. they upgrade to Pike, then to Queens. still no resource_class. | |
| 14:34:39 | dansmith | dtantsur: ah, right, for nodes with an instance | |
| 14:34:41 | mordred | dtantsur: what would the resource_class be if they created it with an empty resource_class today? | |
| 14:34:59 | dtantsur | mordred: None | |
| 14:35:08 | dtantsur | (python None, or JSON null) | |
| 14:35:09 | dansmith | mordred: it's a thing that is required for queens | |
| 14:35:15 | mordred | AH | |
| 14:35:48 | dansmith | dtantsur: so the response probably needs to be "400: You cannot CHANGE the class of an active node" | |
| 14:35:52 | dtantsur | so yeah, it was perfectly valid to not have any resource_class when we introduced it (which is sad, btw) | |
| 14:35:59 | dansmith | dtantsur: sad indeed | |
| 14:36:11 | dansmith | heh | |
| 14:37:22 | dtantsur | my main question is: how will nova react when a node gets a resource_class (without having it previously) | |
| 14:37:39 | dansmith | dtantsur: it'll do the right thing I think.. currently it's just skipping those and logging a warning | |
| 14:37:44 | dansmith | it's the changing that is a problem | |
| 14:37:54 | dansmith | without it set, we report no inventory and don't migrate the instances | |
| 14:37:58 | dansmith | once it's set, we do those things | |
| 14:38:03 | dtantsur | okay, we can ban changing, I think | |
| 14:38:12 | dansmith | the changing of that (or handling the change) is the complicated bit | |
| 14:38:16 | dtantsur | and what above available nodes? do you see any problems with them? | |
| 14:38:24 | jaypipes | mriedem, dansmith: sorry, wrapping up a meetng | |
| 14:38:36 | dansmith | dtantsur: hmm? above available nodes? | |
| 14:38:46 | dansmith | dtantsur: you mean nodes with no instance on them? | |
| 14:38:56 | dtantsur | yep, but available for nova to deploy on | |
| 14:39:02 | dtantsur | (sorry, /me uses our state machine terms) | |
| 14:39:27 | dansmith | dtantsur: well, it's less problematic for sure, but I think the caching in the ironic driver still means there's a race there, which is not great | |
| 14:39:44 | dansmith | dtantsur: but I'd much rather document that as a pitfall as it doesn't require extra code, | |
| 14:40:03 | dansmith | it just means there's a window where you might schedule something with the old class to the node you just changed | |
| 14:40:34 | dtantsur | right. I guess we have the same problems right now with e.g. changing properties (except that people should not change their CPU count too often) | |
| 14:40:35 | dansmith | and you'll migrate the instance to the stale class most likely which will be confusing, but :/ | |
| 14:40:40 | dansmith | I have to brb | |
| 14:40:48 | dtantsur | thanks dansmith, I think I know what to do | |
| 14:42:59 | dtantsur | mmmm, coffee :) | |
| 14:43:25 | edleafe | dtantsur: yeah, if the API already returns a particular code, and what you're changing isn't a thing that people rely on, it's a bugfix, not a version bump | |
| 14:43:36 | edleafe | dtantsur: so yeah, what mordred said | |
| 14:44:11 | jaypipes | mriedem: k, sorry, done now. | |
| 14:44:18 | dtantsur | edleafe: okie, I'll bake a patch today | |
| 14:44:22 | edleafe | dtantsur: if a node has no resource_class set and an instance on it, and then the resource_class gets set, the next time through the instance flavor will get updated | |
| 14:44:29 | jaypipes | mriedem: I'm not entirely sure how one would add tests for a continue block... | |
| 14:44:49 | edleafe | dtantsur: dansmith: that's why the 'seen' cache is keyed on (instance_uuid, rc) | |
| 14:45:48 | mriedem | jaypipes: i guess you'd assert that something *didn't* happen | |
| 14:45:50 | mriedem | since you continued | |
| 14:46:04 | mriedem | but anyway, it can be a follow up given the circumstances with time | |
| 14:46:47 | jaypipes | mriedem: k. answered your comment about local delete.. | |
| 14:47:24 | edleafe | dtantsur: I'll revise that migration patch to remove the support for a node with an instance changing its resource_class | |
| 14:47:42 | dansmith | edleafe: dtantsur thanks a bunch | |
| 14:48:46 | dtantsur | np | |
| 14:55:08 | mriedem | dansmith: jaypipes: is this required for rc1 or just sugar? https://review.openstack.org/#/c/491012/ | |
| 14:55:08 | openstackgerrit | Eric Fried proposed openstack/nova master: nova.utils.get_ksa_adapter() https://review.openstack.org/488137 | |
| 14:55:08 | openstackgerrit | Eric Fried proposed openstack/nova master: Get auth from context for glance endpoint https://review.openstack.org/490057 | |
| 14:55:19 | dansmith | mriedem: required | |
| 14:55:47 | jaypipes | dansmith: to be perfectly frank, I don't know. :( | |
| 14:55:55 | jaypipes | sorry, that was for mriedem ^ | |
| 14:56:12 | jaypipes | mriedem: see the comment I just responded to edleafe and dansmith with. | |
| 14:56:45 | jaypipes | mriedem: basically boils down to "I totally think we probably might need to maybe do something here, but I'm not sure what..." | |
| 14:56:48 | dansmith | jaypipes: huh? before that patch pike computes are behaving like ocata ones | |
| 14:56:56 | dansmith | that's where it was when I left it anyway | |
| 14:58:00 | mriedem | alright i'll review https://review.openstack.org/#/c/488510/ in the meantime then | |
| 14:58:34 | jaypipes | dansmith: well, I guess after going through the scenarios in my head, I'm wondering what we really need to do there. Probably just need another hangout session with you on it. | |
| 14:58:36 | dansmith | jaypipes: L1038 is the critical bit: https://review.openstack.org/#/c/491012/7/nova/compute/resource_tracker.py | |
| 14:58:40 | openstackgerrit | Eric Fried proposed openstack/nova master: Get auth from context for glance endpoint https://review.openstack.org/490057 | |
| 14:58:48 | openstackgerrit | Sean Dague proposed openstack/nova master: Add documentation for documentation contributions https://review.openstack.org/492124 | |
| 14:58:48 | openstackgerrit | Sean Dague proposed openstack/nova master: Clean up *most* ec2 / euca2ools references https://review.openstack.org/492166 | |
| 14:59:04 | dansmith | jaypipes: without that line, we don't move the ball forward with comptues just stomping all over everything on both sides of a boot or move operation | |
| 14:59:24 | jaypipes | dansmith: oh, I totally get that. the part I'm wondering about is whether that "heal allocations" method needs to change now. | |
| 14:59:27 | dansmith | er, without gating that line on the version | |
| 14:59:44 | dansmith | jaypipes: that's not what he was asking, right? he was asking if the whole change is required for rc1 right? | |
| 15:00:02 | jaypipes | dansmith: ah, sorry. mriedem, yes, it is. | |
| 15:00:19 | jaypipes | mriedem: I was -Workflow on it to discuss that bit about the heal allocations. | |
| 15:00:26 | jaypipes | sorry for confusion. | |
| 15:00:31 | mriedem | np | |
| 15:00:56 | dansmith | jaypipes: your non-functional continue in a conditional is part of your extra debug logging I assume, and that was all you so..whatever you want to do there | |
| 15:00:56 | mnaser | so: there should never be a scenario where an instance fails to spawn with local variable referenced before assignment.. right? | |
| 15:01:16 | mriedem | mnaser: correct | |
| 15:01:21 | mnaser | because im trying to go through this code and i cant find how/why it's happening | |
| 15:01:32 | mnaser | okay. i'll go do some more in depth checking then | |
| 15:01:39 | mriedem | UnboundLocalError right? | |
| 15:01:48 | jaypipes | dansmith: heh, yeah... I was tempted to put into the log debug message something like "we're really not sure whether we even get here any more and if we do, what we should do anyway" ;) | |
| 15:01:52 | mriedem | seems that would be obvious | |
| 15:02:09 | mnaser | mriedem the manager is catching the original exception so its making it a tad harder | |
| 15:02:42 | mnaser | based on my search, it should either be in nova/objects/numa.py or nova/virt/hardware.py as those are the two references to it | |
| 15:02:43 | mriedem | oh, LOG.exception? | |
| 15:02:55 | dansmith | jaypipes: yeah, and it might be useful. Just don't say "We were completely burned out at the end of pike and this seems like a bad thing we probably should have handled. Sorry about that." | |
| 15:03:12 | jaypipes | dansmith: don't give me ideas... :P | |
| 15:03:27 | mnaser | mriedem i'll have to do, it's def not catching by default, compute logs is just showing: 2017-08-09 14:33:00.758 4242 DEBUG nova.compute.utils [req-91225f58-71c5-44d3-ae5c-734986b4a3f7 16e021e0ed5f47b68c095d6885f18f4b d7594b0298b54bcc9e4e0f252e1da2e4 - - -] [instance: 18b91008-d42f-4eac-b57d-07195e7774ba] local variable 'sibling_set' referenced before assignment notify_about_instance_usage /usr/lib/python2.7/site-packages/nova/compute/utils.py | |
| 15:03:27 | mnaser | :313 | |
| 15:03:36 | dansmith | mriedem: comment in here about the wording https://review.openstack.org/#/c/491582/6 | |
| 15:03:45 | jaypipes | dansmith: how about this? LOG.debug("We used to think we were indecisive. Now we're not so sure.") | |
| 15:04:07 | dansmith | jaypipes: that seems fine to me. highly truth-based | |
| 15:04:15 | jaypipes | very truthy indeed. | |
| 15:04:15 | dansmith | jaypipes: but I'd only +1 it and wait for others to +2 | |
| 15:07:04 | mriedem | dansmith: replied https://review.openstack.org/#/c/491424/7/releasenotes/notes/pike_prelude-fedf9f27775d135f.yaml - so you want me to add those or leave it? | |
| 15:08:22 | dansmith | mriedem: I didn't realize that wasn't +A.. I would have.. I was just suggesting that maybe circling back and putting it in there might be worthwhile, but it's really not a big deal | |
| 15:08:38 | dansmith | it's on its way now | |
| 15:08:46 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: replace chance with filter scheduler in func tests https://review.openstack.org/491529 | |
| 15:08:48 | dansmith | I was just commenting on your comment | |