| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-15 | |||
| 14:28:21 | mriedem | if you're trapping that error, there will be no fault | |
| 14:28:24 | mriedem | or error notification | |
| 14:28:28 | dansmith | oh | |
| 14:28:32 | dansmith | yeah, mriedem just answered | |
| 14:28:36 | mriedem | you'll only record a fault or send an error notification if something raises, like NotImplementedError | |
| 14:28:58 | mriedem | @wrap_instance_event will record a start and finish event on entry and exit of the method regardless of how it exits | |
| 14:29:10 | dansmith | you can log and re-raise maybe? | |
| 14:29:10 | mriedem | the event gets record on the instance action (Created in the api) keyed by instance uuid and context.request_id | |
| 14:29:19 | mriedem | *recorded | |
| 14:29:52 | tssurya | mriedem: yea but since I do a return from the preemption if any it records a success I guess ? | |
| 14:30:32 | mriedem | yes, it will say it was successful | |
| 14:30:51 | mriedem | https://github.com/openstack/nova/blob/872a823d9a02f31e266882bfb499673c51fb4075/nova/compute/utils.py#L1302 | |
| 14:31:13 | mriedem | https://github.com/openstack/nova/blob/872a823d9a02f31e266882bfb499673c51fb4075/nova/objects/instance_action.py#L184 | |
| 14:31:36 | aspiers | efried: *finally* figured it. patch(..., new_callable=mock.PropertyMock), *not* patch(..., new_callable=mock.PropertyMock(name='fake_prop')) | |
| 14:31:41 | mriedem | i think i mentioned something in my earlier big round of comments that the functional test should assert the events are recorded as expected for the compute_power_update event | |
| 14:32:00 | tssurya | mriedem: yea I added that | |
| 14:32:20 | tssurya | but this should not be a success for the task_state race error right ? | |
| 14:32:38 | mriedem | idk | |
| 14:32:52 | mriedem | yesterday i thought we said that we'd handle UnexpectedTaskStateError, log and return | |
| 14:33:02 | mriedem | if you want to record the action as failed, then you'd log and re-raise | |
| 14:33:08 | yonglihe | mriedem: ok if i need to bump up minor version, i gonna do that, but thanks. | |
| 14:33:13 | tssurya | ok yea | |
| 14:33:18 | tssurya | I'll do that | |
| 14:33:23 | dansmith | mriedem: I didn't grasp that detail about letting it go | |
| 14:33:24 | mriedem | well, what does dansmith think | |
| 14:33:27 | mriedem | oh | |
| 14:33:39 | mriedem | note that ironic at this point has got a response and moved on | |
| 14:33:52 | dansmith | ironic doesn't care, | |
| 14:33:54 | mriedem | so this is only useful for an admin debugging that power-update exchange and if it worked | |
| 14:33:56 | dansmith | this is more like what a user would see | |
| 14:34:03 | dansmith | or the user itself right? | |
| 14:34:12 | tssurya | yea | |
| 14:34:20 | mriedem | the user can see the actions on their server yeah | |
| 14:34:29 | tssurya | I am also concerned because of the instance-action CLI show thing | |
| 14:34:32 | efried | aspiers: That's a facet of new= vs new_callable= | |
| 14:34:35 | mriedem | and the events after a certain microversion (but not traceback details unless they are admin) | |
| 14:34:46 | aspiers | efried: yeah, I realise that *now*, after two hours :-( | |
| 14:34:48 | yonglihe | dansmith: I need to bother you with it then. https://review.opendev.org/#/c/621476/ | |
| 14:34:48 | dansmith | I can see it both ways: | |
| 14:35:03 | aspiers | efried: I was misled by some existing nova tests which use it incorrectly | |
| 14:35:04 | dansmith | either they see an action that started/finished, but nothing happened because it was basically dropped, which might be weird, | |
| 14:35:09 | efried | aspiers: If you need to customize your PropertyMock, you would use new=PropertyMock(...) | |
| 14:35:14 | dansmith | and they might see an action start/fail and think something is wrong | |
| 14:35:15 | dansmith | so I dunno | |
| 14:35:21 | dansmith | mriedem: tssurya ^ | |
| 14:35:30 | aspiers | it can't possibly be me just being stupid | |
| 14:35:42 | efried | aspiers: Feel free to propose a patch to fix those bogus usages | |
| 14:35:48 | tssurya | dansmith: yea let me check how it is for other actions like pause | |
| 14:35:48 | aspiers | efried: I certainly will | |
| 14:35:55 | efried | I see three-ish | |
| 14:35:57 | tssurya | which are not implemented in ironic driver | |
| 14:36:10 | tssurya | wonder if that records a success action | |
| 14:36:18 | openstackgerrit | Chris Dent proposed openstack/nova master: DNM: add profiling to scheduler https://review.opendev.org/674810 | |
| 14:36:45 | mriedem | tssurya: it wouldn't, | |
| 14:36:50 | mriedem | https://github.com/openstack/nova/blob/872a823d9a02f31e266882bfb499673c51fb4075/nova/compute/manager.py#L4977 | |
| 14:36:51 | tssurya | yea it doesn't | |
| 14:36:57 | mriedem | it would record a failure b/c of NotImplementedError | |
| 14:37:02 | tssurya | yea | |
| 14:38:00 | tssurya | dansmith, mriedem: if you are both ok with the whole success and return thing I can do that | |
| 14:38:16 | tssurya | but if you feel its confusing for the users I can reraise and log and ignore raise | |
| 14:38:34 | dansmith | tssurya: I dunno, that I have a strong feeling about which is right or not | |
| 14:39:23 | tssurya | dansmith: ack | |
| 14:39:38 | mriedem | gmann: then why have these unit tests still? https://review.opendev.org/#/c/676275/4/novaclient/tests/unit/v2/test_shell.py | |
| 14:39:53 | mriedem | they are just asserting what the stub is doing https://review.opendev.org/#/c/676275/4/novaclient/tests/unit/v2/fakes.py | |
| 14:39:59 | mriedem | which might be wrong if there is a bug in the stub | |
| 14:41:11 | mriedem | idk either - the user is going to see a stop or start action with a compute_power_update event, they likely won't know what that event means. does the user know that someone powered on or off their bm instance out-of-band? | |
| 14:41:57 | mriedem | and since this isn't an action initiated by the user (at least directly in the compute api) but by ironic, is it just best effort and we shouldn't record a failed event if that operatoin lost a race? | |
| 14:42:32 | mriedem | if it's best effort and we don't consider losing a race a failure, then we shouldn't record the overall action as failed, but just "processed" | |
| 14:42:59 | dansmith | mriedem: yeah, that's a valid way to look at it I think | |
| 14:43:14 | dansmith | it's best effort, a hint so to speak, so failure isn't really failure | |
| 14:43:19 | tssurya | fair enough | |
| 14:43:36 | tssurya | and its easier that way for me :) | |
| 14:43:56 | dansmith | just put "it's easier for tssurya" in the spec and then you're good ;) | |
| 14:43:59 | tssurya | I'll stick to what we discussed yesterday then | |
| 14:44:06 | tssurya | dansmith: hehe | |
| 14:44:09 | mriedem | thinking about volume-extended events, we definitely want those to be failed so the user sees because it's initiated by the user resizing their volume in the cinder api | |
| 14:44:25 | tssurya | mriedem: this was a another doubt I had | |
| 14:44:33 | tssurya | that means we block the rest of the events in the queue no ? | |
| 14:44:42 | tssurya | because one volume-extend event broke ? | |
| 14:44:50 | tssurya | and you raise | |
| 14:45:02 | mriedem | we wouldn't get a volume-extended and power-update event in the same request | |
| 14:45:10 | mriedem | unless someone sends that manually | |
| 14:45:34 | mriedem | there might be a bug in the volume-extended event handling in compute that's not handling that properly, but that's a separate bug | |
| 14:45:50 | tssurya | mriedem: got it | |
| 14:46:51 | mriedem | tssurya: how much longer are you going to be around today? | |
| 14:46:54 | tssurya | mriedem: do you have a sample functional test that you have written for such races | |
| 14:46:56 | tssurya | ? | |
| 14:47:13 | tssurya | I couldn't find a way to hack the task_state in between my code | |
| 14:47:16 | tssurya | for the functional teste | |
| 14:47:35 | tssurya | mriedem: i'll be around only for 2 more hours | |
| 14:47:36 | mriedem | you'd have to stub/mock something in between to change the task_state after the compute manager sets it and the driver code gets it | |
| 14:47:50 | mriedem | i'm not sure we need the functional test to cover that though | |
| 14:48:04 | mriedem | that's pretty unit-y test level to me | |
| 14:48:10 | tssurya | but its ok, I'll be online and if you have comments will update them during vacation | |
| 14:48:45 | tssurya | mriedem: ok then I'll just do a unit test | |
| 14:49:16 | dansmith | yep, unit test makes sense to me | |
| 14:49:23 | dansmith | just to make sure we catch it and do the right thing | |
| 14:49:51 | tssurya | dansmith: yea I'll do that | |
| 14:51:19 | mriedem | tssurya: but i won't be around to review the updates until monday... and people are held up on this microversion, so i'll review it in detail now before you make another push | |
| 14:52:08 | tssurya | mriedem: ok sounds good | |