| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-11 | |||
| 13:08:14 | openstack | mriedem: Error: "=" is not a valid command. | |
| 13:17:08 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/queens: Fix TypeError in prep_resize allocation cleanup https://review.openstack.org/581741 | |
| 13:17:20 | mriedem | gibi: i think you might have officially made yourself the other core to review https://review.openstack.org/#/c/563375/ | |
| 13:17:32 | mriedem | dansmith would +2 it but he made some non-trivial changes to it | |
| 13:19:56 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] add error.code on a ConcurrentUpdateDetected https://review.openstack.org/581742 | |
| 13:21:25 | mriedem | gibi: btw, you've been quite about reviews on the bw resource provider series - what's the status on that? are you looking for reviews on incremental things? is it done end to end or still working on changes for the full stack in nova (and maybe deps in neutron)? | |
| 13:21:55 | mriedem | my giant port binding live migration series is basically blocked on neutron dependencies | |
| 13:24:29 | openstackgerrit | Chris Dent proposed openstack/nova master: Test for unsanitized consumer UUID https://review.openstack.org/581137 | |
| 13:28:53 | gibi | mriedem: I will go through the remainings of https://review.openstack.org/#/c/563375/ soon | |
| 13:29:23 | gibi | mriedem: the status of bandwidth is that the code up on review stopped working after the official support for nested allocation_candidate support is merged | |
| 13:29:38 | gibi | mriedem: the solution is to support the new placement API versions from the scheduler report client | |
| 13:30:28 | gibi | mriedem: for that I started with a failing functional test here https://review.openstack.org/#/c/527728/ | |
| 13:31:05 | mriedem | yikun: gmann: alex_xu: so i see there must have been some discussion on https://review.openstack.org/#/c/567534/ and the request format wasn't changed but the response fields were changed...but no comments in the review about that decision? | |
| 13:37:32 | alex_xu | mriedem: no decision yet, I will try to go through the review | |
| 13:38:15 | alex_xu | mriedem: do you remember whether we discuss what should we do if the image traits check failed for rebuild https://review.openstack.org/#/c/569498/ ? | |
| 13:43:09 | mriedem | alex_xu: glancing at the comments, the question is if the instance should be in ACTIVE or ERROR state? | |
| 13:43:23 | alex_xu | mriedem: yea, that is the question | |
| 13:43:56 | mriedem | if we hit the scheduler during rebuild today because the image changes, and we get NoValidHost, we put the instance into ERROR state | |
| 13:44:22 | mriedem | so i'd expect we do the same if the new image traits are invalid for the instance.host - which is essentially the same thing as a NoValidHost we'd get from the scheduler | |
| 13:45:31 | mriedem | you can rebuild an instance in ERROR state btw | |
| 13:45:57 | alex_xu | oh, we can rebuild an instance in error, that sounds better | |
| 13:46:11 | mriedem | https://review.openstack.org/#/q/Ibb7bee15a3d4ee6f0ef53ba12e8b41f65a1fe999 | |
| 13:46:23 | mriedem | setting the instance to ERROR is a relatively new change | |
| 13:46:30 | alex_xu | otherwise I feel the user just chocie a wrong image, then his instance get into error, he only can get help from admin | |
| 13:47:09 | mriedem | they can rebuild with a good image | |
| 13:47:21 | mriedem | i remember this change and brought it up during the ptg in dublin | |
| 13:47:28 | mriedem | discussing maybe re-architecting how rebuild works, | |
| 13:47:37 | mriedem | the reason we set it to ERROR state is not just because scheduling failed, | |
| 13:47:47 | mriedem | but also because we've changed values on the instance itself in the api before casting to conductor, | |
| 13:47:54 | mriedem | so at that point, we can't rollback those changes | |
| 13:48:06 | mriedem | see my comments in https://review.openstack.org/#/c/536268/ | |
| 13:50:34 | alex_xu | mriedem: ah, i see now, that problem is out of the scope https://review.openstack.org/#/c/569498/ | |
| 13:51:09 | mriedem | yup | |
| 13:51:17 | mriedem | i need to review that rebuild + image traits change too | |
| 13:51:28 | mriedem | since i'm the one that got it stuck in committee during spec review | |
| 13:52:34 | mriedem | dansmith: so you don't see a need to name the new fields in the server group API policy_name and policy_rules right? | |
| 13:52:39 | alex_xu | mriedem: yea | |
| 13:52:55 | dansmith | mriedem: I don't, but gmann said he thought it was important | |
| 13:52:55 | mriedem | because yikun re-wrote the api change to do that, which makes it inconsistent with both the notification payload and the internal object modeling | |
| 13:53:06 | mriedem | i don't think it's important or really confusing | |
| 13:53:11 | dansmith | me either | |
| 13:53:18 | mriedem | i think it's more important that we have consistency up and down the stack | |
| 13:53:20 | mriedem | internal and external | |
| 13:57:26 | openstackgerrit | Eric Fried proposed openstack/nova master: Delete orphan compute nodes before updating resources https://review.openstack.org/579922 | |
| 14:03:22 | mriedem | gibi: just ping me if / when you need reviews on the bw provider changes | |
| 14:12:33 | openstackgerrit | Merged openstack/nova master: Remove irrelevant comment https://review.openstack.org/578821 | |
| 14:16:11 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Update queued-for-delete from the ComputeAPI during deletion/restoration https://review.openstack.org/566813 | |
| 14:33:00 | gibi | mriedem: thanks. | |
| 14:34:42 | efried | alex_xu: Are you still -1 on that change? Given that we've concluded NoValidHost is the right thing, having those logs be ERROR seems appropriate, 对吧 | |
| 14:37:30 | mriedem | efried: we don't ERROR for NoValidHost in the scheduler | |
| 14:37:41 | mriedem | ERROR means the operator needs to investigate b/c there is a problem in the system, | |
| 14:37:48 | mriedem | in this case, it's really a user error like a 400 | |
| 14:37:51 | mriedem | we don't log errors for that | |
| 14:37:56 | mriedem | debug at most | |
| 14:38:34 | efried | mriedem: So in this case, how would the user know what went wrong? | |
| 14:39:14 | mriedem | do they get a fault recorded? | |
| 14:39:55 | mriedem | set_vm_state_and_notify will record a fault on the instance for the NoValidHost, | |
| 14:40:19 | mriedem | and the user should be able to see the fault message for the error | |
| 14:40:57 | mriedem | https://developer.openstack.org/api-guide/compute/faults.html#instance-faults | |
| 14:41:27 | mriedem | which reminds me, you can only see faults for ERROR or DELETED servers, so that's another reason to set the server to ERROR in this case so they can see the fault | |
| 14:41:56 | mriedem | so as long as the NoValidHost has a reasonable message we should be fine | |
| 14:42:17 | mriedem | that would be a good wrinkle for the functional test though right? | |
| 14:43:06 | openstackgerrit | do3meli proposed openstack/nova master: docs: add nova host-evacuate command to evacuate documentation https://review.openstack.org/578040 | |
| 14:44:08 | mriedem | why is all this shared provider stuff in this change? https://review.openstack.org/#/c/569498/12/nova/tests/functional/libvirt/test_shared_resource_provider.py | |
| 14:44:49 | efried | mriedem: I asked for it. | |
| 14:45:14 | efried | mriedem: Because it's important that we're checking all and only providers involved in the allocation. | |
| 14:45:39 | efried | mriedem: So we should also be checking nested, but letting that slide for now because we don't have any scenarios that actually use nested yet. | |
| 14:46:33 | mriedem | hmm, ok. | |
| 14:46:41 | mriedem | and those unit tests look to be completely redundant for conductor | |
| 14:46:47 | mriedem | redundant with the functional tests | |
| 14:46:51 | mriedem | we don't need both for the same scenarios | |
| 14:47:17 | mriedem | https://review.openstack.org/#/c/569498/12/nova/tests/functional/libvirt/test_shared_resource_provider.py is just a really weird location for rebuild tests... | |
| 14:48:28 | efried | if it's testing how shared resource providers are handled by rebuild by libvirt, it seems as good as anything. | |
| 14:49:34 | mriedem | the validation being added to conductor is virt-agnostic | |
| 14:49:42 | mriedem | so it's a weird place to have those tests | |
| 14:49:43 | mriedem | is all | |
| 14:50:17 | efried | but libvirt is the only driver that's doing shared providers (properly) at the moment. | |
| 14:50:31 | mriedem | yeah i know, but that doesn't really matter here does it? | |
| 14:50:39 | mriedem | it's just a convenient place to put the tests given the setUp | |
| 14:51:01 | mriedem | we could have just as easily written a functional test that populate providers and allocations in placement and then run rebuild | |
| 14:51:07 | mriedem | anyway, i'm not -1 on it, it's just weird | |
| 14:51:23 | efried | Agree the main motivator was probably convenience as you say. | |
| 14:57:49 | mriedem | i'd like to nuke all of these checks http://codesearch.openstack.org/?q=We%20need%20to%20mock%20that%20the%20old%20way&i=nope&files=&repos= | |
| 14:57:52 | mriedem | seems we can do that now right? | |
| 14:58:16 | mriedem | oh nvm, we haven't dropped the migrate_instances_add_request_spec online data migration yet | |
| 14:59:17 | mriedem | dansmith: it's probably not very possible to do a blocker migration for the request spec migration is it? | |
| 14:59:31 | mriedem | since it's api db + multi-cell | |
| 14:59:46 | mriedem | we could add a nova-status check pretty easily | |
| 14:59:59 | mriedem | iterate all cells and make sure all non-deleted instances have a request spec | |
| 15:00:00 | dansmith | for which migration? | |
| 15:00:04 | dansmith | ah | |
| 15:00:05 | dansmith | yeah | |
| 15:00:07 | mriedem | migrate_instances_add_request_spec | |
| 15:00:21 | mriedem | yeah - i want to kill all of these "if not request_spec: populate from garbage" in the code | |
| 15:00:34 | dansmith | hah yeah | |
| 15:00:42 | mriedem | we can't do a blocker db migration in db sync | |
| 15:00:54 | mriedem | well, shouldn't | |
| 15:00:55 | mriedem | ? | |
| 15:01:08 | mriedem | but nova-status could do the job, i mean this migration has been around since newton | |
| 15:01:13 | dansmith | I think people have also expressed displeasure with the blocker migrations, so any time we're not enforcing some really structural thing (like a column being non-null or something) a nova-status would be better | |