| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-09 | |||
| 08:34:21 | gibi | good morning | |
| 09:05:35 | jianghuaw | Is the DB nova-api allowed to be accessed by nova-compute service? | |
| 09:07:47 | jianghuaw | I met an error as "RemoteError: Remote error: CantStartEngineError No sql_connection parameter is established" when nova-compute tries to query data from aggregate which belong to nova-api db. | |
| 14:00:52 | openstackgerrit | Matthew Edmonds proposed openstack/nova master: update policy UT fixtures https://review.openstack.org/398610 | |
| 14:12:19 | mriedem | i'm going through https://review.openstack.org/#/c/491850/ now | |
| 14:13:22 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Improve stable-api doc with current API state https://review.openstack.org/489926 | |
| 14:18:52 | openstackgerrit | Maciej Jozefczyk proposed openstack/nova master: Remove host filter for _cleanup_running_deleted_instances periodic task https://review.openstack.org/491808 | |
| 14:20:29 | openstackgerrit | Matthew Edmonds proposed openstack/nova master: use conf for keystone session creation https://review.openstack.org/485121 | |
| 14:21:45 | dtantsur | dansmith: hi! re your comment on the ironic-related patch: should we block changing node.resource_class for active nodes in Ironic? | |
| 14:22:07 | dansmith | dtantsur: if that's possible I think that would be an excellent idea | |
| 14:22:50 | dtantsur | dansmith: it's not impossible, but our beloved API microversion will kick in here. meaning, we'll only be able to block it starting with the next API version :( | |
| 14:23:13 | dtantsur | I would actually block it in all versions, given that it's going to screw up nova | |
| 14:23:25 | dansmith | dtantsur: presumably you can return a 409 for mostly any reason right? | |
| 14:23:37 | dtantsur | but people tend to feel quite religiously about not bypassing the versioning | |
| 14:24:00 | dtantsur | dansmith: right, but how does it help? | |
| 14:25:22 | dtantsur | it's an interesting corner case of the API WG to discuss. should we leave a feature that clearly breaks things, or should we break the versioning contract | |
| 14:25:46 | dansmith | dtantsur: well can you return 409 for anything else in that call? | |
| 14:26:14 | dtantsur | dansmith: sorry, I think I don't get the question. We cannot just randomly return 409 I think.. | |
| 14:27:02 | mriedem | dtantsur: mearning, can the node update api return a 409 already for something else | |
| 14:27:09 | dansmith | dtantsur: if 409 is already a valid return value then I think it's less problematic | |
| 14:27:10 | dansmith | right | |
| 14:27:11 | mriedem | so the user can already be expecting a 409 in some cases | |
| 14:27:34 | dansmith | and I would expect 409 to be valid for most PUTs to cover situations like this | |
| 14:27:41 | dansmith | like "you're violating some constraint" | |
| 14:27:45 | dtantsur | mriedem, dansmith, I think I get where you're heading. Yes, we can. And no, according to the API versioning ideology, we cannot do it. | |
| 14:28:03 | dtantsur | it's not only about breaking users, it's more about signaling changes /me waits for mordred to jump in | |
| 14:28:13 | mordred | so... | |
| 14:28:13 | mriedem | https://developer.openstack.org/api-ref/baremetal/#update-node | |
| 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 | dansmith | mordred: agreed | |
| 14:30:57 | dtantsur | however, the generic 400 an also be returned from essentially any endpoint, we can use it | |
| 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 | |