Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-11
12:54:45 gibi mriedem: I think the api accepts strings for historical reasons
12:55:33 gibi mriedem: I sure I'm OK to cast it to int internally for counting and comparison
12:57:08 mriedem ok. i just don't understand the json schema validation concern
12:57:19 mriedem is that a concern for the consumer of the notification?
12:59:01 gibi mriedem: no it is not. I refer to gmann's comment in https://review.openstack.org/#/c/563401/28/nova/notifications/objects/server_group.py@41 about the need to move the validation from json to code
13:01:19 mriedem oy
13:01:36 mriedem there is only 1 policy per group, and the rules are applied to that policy on that group,
13:01:46 mriedem we've already named the InstanceGroup.rules field
13:03:58 gibi yeah that is gmann's point
13:03:59 mriedem which i think is ok, it's like a 2 line validator
13:04:12 gibi I agree
13:04:15 mriedem if group.policy == 'anti-affinity' and group.rules: raise HTTPBadRequest
13:04:23 openstackgerrit Stephen Finucane proposed openstack/nova master: Revert "docs: Disable smartquotes" https://review.openstack.org/578841
13:04:23 mriedem oops
13:04:29 mriedem != 'anti-affinity'
13:05:04 gibi sure
13:05:30 gibi policy_rule feels a better name for me but I accept rule with a good api doc
13:06:51 openstackgerrit sahid proposed openstack/nova stable/queens: hardware: fix hugepages memory usage per intances https://review.openstack.org/581736
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 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:52:55 dansmith mriedem: I don't, but gmann said he thought it was important
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.

Earlier   Later