| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-15 | |||
| 14:20:30 | openstackgerrit | Chris Dent proposed openstack/nova master: single pass instance info fetch in host manager https://review.opendev.org/623558 | |
| 14:20:40 | mriedem | yonglihe: i can review again based on my previous comments but i likely won't be approving that change, because (1) we're trying to land tssurya's change for 2.76 since there is an approved ironic change that depends on the nova change (see the ML) and (2) i can't vote on the low-level contents of what's going into the numa topology response, i think dansmith or others that were more involved in the spec approval should revie | |
| 14:20:40 | mriedem | ose details. | |
| 14:21:08 | mriedem | sean-k-mooney: ack on the lxc thing; we can define an image to use in the job, but if that hard-coding in the libvirt driver is a problem we'll need to do something about that. | |
| 14:21:42 | mriedem | sean-k-mooney: i'm not sure how much we care about cloud-init in the lxc job, but someone does have some patches up for making cloud-init work with lxc (so saying that, it might not work today anyway) | |
| 14:21:56 | tssurya | dansmith: ah nvm, I got the answer | |
| 14:21:58 | mriedem | sean-k-mooney: yeah this https://review.opendev.org/#/c/667976/ | |
| 14:22:05 | tssurya | :D sorry for the noise | |
| 14:22:13 | openstackgerrit | Eric Fried proposed openstack/nova master: Bump minimum ksa (3.16.0) and sdk (0.34.0) https://review.opendev.org/676707 | |
| 14:22:13 | dansmith | tssurya: heh, okay, sorry | |
| 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 | aspiers | but it just returns a mock object instead | |
| 14:24:25 | sean-k-mooney | dansmith: sorry context are you refering to stephens chagnes? | |
| 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 | 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 | |