| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-09 | |||
| 14:28:13 | mriedem | https://developer.openstack.org/api-ref/baremetal/#update-node | |
| 14:28:13 | mordred | so... | |
| 14:28:17 | mriedem | doesn't mention error codes | |
| 14:28:18 | dtantsur | (for the record: I'm not the biggest fan of the API versioning here) | |
| 14:28:30 | mordred | for signalling changes, it's to signal whether someone can do something or not | |
| 14:28:38 | dansmith | dtantsur: well, regardless of whether ironic lets you do it, the rule to operators should be that they will break stuff if they do it once things are populated | |
| 14:28:41 | mordred | the thing in this case isn't a valid thing to try to do | |
| 14:28:58 | mordred | so a user who does it today isn't actually doing a thing that works | |
| 14:29:13 | mordred | so they don't actually have, you know, an application that is going to break when you do this | |
| 14:29:28 | dtantsur | fair | |
| 14:29:58 | dansmith | that's true, although the application here is *probably* their ansible playbook | |
| 14:29:59 | mriedem | you've got a 409 right here https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L1698 | |
| 14:30:07 | mordred | alternately, they don't need to ask the API if they can avoid doing the bad state -they can, as a client, avoid writing broken code without setting a microversion, since this is mostly about ironic returning an error when they do something stupid | |
| 14:30:09 | mordred | SO | |
| 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. | |