| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-09 | |||
| 14:30:19 | dansmith | where they've set a bunch of nodes to be a given class and will blindly blow that into ironic, I'm guessing | |
| 14:30:36 | mordred | I'd say this is a bug fix where requiring a version bump does't actually add value to anyone | |
| 14:30:37 | dtantsur | mriedem: let's please not use 409 though. we've done a big mistake in our youth, and now ironicclient retries it | |
| 14:30:51 | dansmith | eww | |
| 14:30:57 | dtantsur | however, the generic 400 an also be returned from essentially any endpoint, we can use it | |
| 14:30:57 | dansmith | mordred: agreed | |
| 14:31:07 | dtantsur | mordred: I like the way you put it :) thanks! | |
| 14:31:27 | mordred | dtantsur: I will happily advocate for the validity of this change not actually being an API break if you need me to | |
| 14:31:39 | mordred | rules are there to help us do the right thing - they're not there for their own sake | |
| 14:31:45 | dtantsur | right | |
| 14:32:20 | dtantsur | next tricky question :) | |
| 14:32:45 | dtantsur | what should we do about nodes that do not have any resource_class so far? I mean, active nodes? | |
| 14:32:57 | dtantsur | dansmith: ^^ | |
| 14:33:13 | dansmith | dtantsur: if you take the strict meaning of mordred's comments above, | |
| 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? | |