| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-15 | |||
| 14:22:16 | cdent | mriedem: I reckon that single pass instance info thing ^ is a real winner. I've removed some dead code (I think jay had future plans) but I'm a bit lost on what to do test wise, so if you have ideas, I've got ears. | |
| 14:22:21 | sean-k-mooney | "/sbin/init" has been the location of the init script for decades so i think cirros just need to add a symlink | |
| 14:22:29 | dansmith | sean-k-mooney: I have that open in a tab, working on trying to focus on it for a bit... is there a change coming? | |
| 14:22:34 | sean-k-mooney | im more worried about the symlink | |
| 14:22:40 | sean-k-mooney | *filesystem | |
| 14:22:59 | sean-k-mooney | e.g not resizing it before starting the container | |
| 14:23:20 | sean-k-mooney | i dont know if you can resize it inside the container and a spent a while trying to do it last night and failed | |
| 14:23:34 | efried | aspiers: what are you trying to do with PropertyMock? | |
| 14:23:50 | aspiers | trying to patch objects.Instance.image_meta to return my image_meta fixture | |
| 14:24:24 | mriedem | cdent: fwiw track_instance_changes might be true in grenade jobs, would need to check, but grenade jobs don't run in superconductor mode | |
| 14:24:25 | sean-k-mooney | dansmith: sorry context are you refering to stephens chagnes? | |
| 14:24:25 | aspiers | but it just returns a mock object instead | |
| 14:24:30 | sean-k-mooney | or something else | |
| 14:24:44 | dansmith | sean-k-mooney: gah, sorry that was supposed to be aimed at tssurya | |
| 14:24:50 | sean-k-mooney | ah | |
| 14:24:51 | aspiers | calling instance.image_meta() returns the fixture I want, but not without the () | |
| 14:24:59 | tssurya | dansmith: ok no wait I didn't get the answer :( . So in the API we do the action_start thing which creates the instance-action event, however when we specially handle the UnexpectedTaskStateError (so that we can continue to process other events) in the manager, the "compute_power_update' event is recorded as a success | |
| 14:25:00 | cdent | mriedem: there's something in devstack comments about that, lemme refresh memory | |
| 14:25:10 | aspiers | it's infuriating, been staring for hours and can't see what I'm doing wrong | |
| 14:25:13 | tssurya | which is wrong right ? | |
| 14:25:31 | dansmith | tssurya: ah, yeah, can we record that as fail instead in the exception handler? | |
| 14:25:38 | mriedem | cdent: having said that, i'm crunched for time today, leaving this afternoon for a long weekend | |
| 14:25:59 | sean-k-mooney | mriedem: ill look at the cloud-init patch but without it we are not doing the ssh key injection or creating a user so you would have to build all that into the image | |
| 14:26:00 | cdent | i don't think there's any rush on this, just think it is definitely worth bring back from the dead | |
| 14:26:02 | tssurya | dansmith: yea probably, let me check what does that | |
| 14:26:29 | dansmith | tssurya: it would be super good if we can get mriedem's ack on that patch before he leaves and then you leave | |
| 14:26:50 | tssurya | wait mriedem is also going on vacation soon ? | |
| 14:27:06 | dansmith | for a day yeah | |
| 14:27:40 | tssurya | hmm, ok I am just a bit confused with instance faults/actions/error notification handling for the UnexpectedTaskStateError part | |
| 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 | mriedem | the event gets record on the instance action (Created in the api) keyed by instance uuid and context.request_id | |
| 14:29:10 | dansmith | you can log and re-raise maybe? | |
| 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 | dansmith | I can see it both ways: | |
| 14:34:48 | yonglihe | dansmith: I need to bother you with it then. https://review.opendev.org/#/c/621476/ | |
| 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 | aspiers | efried: I certainly will | |
| 14:35:48 | tssurya | dansmith: yea let me check how it is for other actions like pause | |
| 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 | |