Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-20
14:02:59 cdent gibi: Again, using the strict interpretation, the older microversions should continue to accept
14:04:03 gibi cdent: assume for a second that we don't make it strict and allow failing in the old microversion. This would mean that there would be no difference between the old and the new microversion
14:04:34 gibi cdent: the only difference would be in the documentation
14:04:41 gibi cdent: about the meaning of the accpet
14:04:53 edleafe alex_xu_: "preferred" doesn't mean anything in placement, since it doesn't order anything
14:05:01 gibi cdent: in this case do we need the microversion bump just for the doc?
14:05:13 edleafe "required" implies a filtering, which is what placement does
14:05:54 alex_xu_ edleafe: 'preferred' order the number of traits which the RP has
14:07:15 cdent gibi: assuming that, then I wouldn't think a microverison was required for a doc change, because the behavior is the same everywhered, right? But if there is in fact a behavior change (in the API itself), that seems like a microversion, right?
14:08:11 alex_xu_ or you guys refer to the implement problem
14:09:45 edleafe alex_xu_: if I prefer a trait, I will accept an RP that doesn't have that trait if necessary. So Placement will return all RPs that satisfy the requirements. It is then up to the scheduler to sort them based on various things, and preferred traits would be one of those things
14:10:32 gibi cdent: I'm hesitant. The external behaviour will change as nova will reject a previously accepted the request. But even if nova today accept such request nova actually lies and nova does not fulfill the request properly as nova does not consider the QoS bandwidth policy on the port that is included in the request during the scheduling
14:11:21 gibi cdent: so by rejecting such request nova we clean up a lie
14:11:23 alex_xu_ edleafe: oh, the weigher will change the order again
14:11:29 cdent gibi: and you break working code
14:11:35 gibi cdent: true
14:11:59 cdent gibi: Which is what microversions are supposed to prevent, even though it means broken (in other ways) client code gets to continue existing
14:12:11 cdent I agree that it is a tricky problem and your hesitancy is warranted
14:12:23 cdent Do you feel the "lie" is a security problem?
14:13:05 gibi cdent: no, the lie is not a security issue. It is just a resource allocation issue
14:13:29 edleafe gibi: would you call the "lie" a bug?
14:13:39 edleafe IOW, Nova isn't doing what it's supposed to?
14:14:03 gibi edleafe: Nova missing support for including the port QoS policy in the placement decision
14:14:38 edleafe gibi: understood. What I'm asking is if nova not doing that is a bug
14:14:51 edleafe or is it just a feature that has not yet been implemented
14:15:01 gibi edleafe: it is a missing feature, not a bug.
14:15:09 gibi edleafe: it never worked before
14:15:15 edleafe gibi: ok, then it definitely needs a microversion
14:16:04 gibi edleafe, cdent: thank you for the discussion. I will link this discussion to the spec
14:16:26 mriedem gibi: "The external behaviour will change as nova will reject a previously accepted the request. But even if nova today accept such request nova actually lies and nova does not fulfill the request properly as nova does not consider the QoS bandwidth policy on the port that is included in the request during the scheduling" - same story with rebuilding a volume-backed instance with a new image; we used to accept that and just n
14:16:26 mriedem onor it, but changed it to a 400 in the API in queens
14:16:39 alex_xu_ each microversion discussion is a war
14:17:25 edleafe alex_xu_: https://twitter.com/EdLeafe/status/976100354650791936
14:17:40 mriedem not really
14:17:52 alex_xu_ edleafe: :)
14:18:42 gibi mriedem: so there is precedence not to bump microversion in this case even if it breaks client code
14:19:19 gibi edleafe, alex_xu_: :)
14:20:14 mriedem gibi: https://review.openstack.org/#/c/520660/
14:20:36 mriedem like most things, it's a case by case basis, but in the case of ^ we said it wasn't a microversion because it was a silent failure on the compute side
14:20:50 mriedem you shouldn't have to opt into being not broken
14:21:13 mriedem but i'm sure with enough time and examples we can spin that all ways
14:22:27 openstackgerrit Chris Dent proposed openstack/nova master: DNM: Demo code for microversion parse extraction https://review.openstack.org/550265
14:23:29 edleafe mriedem: yeah, that was why I was asking if it was a bug or a new feature. You shouldn't have to opt into bug fixes.
14:23:58 alex_xu_ mriedem: https://review.openstack.org/#/c/520660/ is a bug, I feel it is different
14:24:11 gibi mriedem, cdent, edleafe: in the bfv rebuild case that rebuild has never worked before
14:24:18 openstackgerrit Silvan Kaiser proposed openstack/nova master: Exec systemd-run with privileges in Quobyte driver https://review.openstack.org/554195
14:24:29 alex_xu_ gibi: the qos policy works with neutron currently, right?
14:25:05 gibi alex_xu_: the qos policy without placement support cannot really work properly even if neutron implemented some data plane enforcement for that policy
14:26:01 mriedem gibi: depends on what you mean by 'worked',
14:26:14 mriedem the api used to accept that kind of request and return a 202, and nothing would fail, but it was wrong
14:26:26 mriedem wrong in that it didn't actually replace the root disk
14:26:38 gibi alex_xu_: imagine that you promised 10G minimum bandwidth for two VMs on the same host and on the same 10G PF. That will cause inconsistency when both VM start using that minimum bandwidth heavily
14:26:53 mriedem today you can create a port with a qos policy and attach it to an instance, right? nova allows that w/o changing any resource consumption for that qos policy.
14:27:02 gibi mriedem: right
14:27:27 mriedem if we followed the volume-backed rebuild + new image path here,
14:27:41 mriedem we'd merge a patch to check if the port (or network) has a qos policy and fail if so
14:27:53 gibi yes
14:27:57 mriedem and then land a microversion that allows supporting those types of ports
14:28:12 gibi mriedem: wait a bit
14:28:18 mriedem which is what https://review.openstack.org/#/c/532407/ is for
14:29:00 mriedem thinking about volume multiattach, we kind of have the same situation - you can't attach a multiattach volume with microversion < 2.60, you have to opt into using those types of volumes
14:29:00 gibi mriedem: we talking about adding microversion bump to the first patch where we start rejecting such requests not just microversion bump when we later start supporting such request
14:29:31 bauzas sean-k-mooney[m]: looking at https://review.openstack.org/#/c/545951/2/specs/rocky/approved/enable-sriov-nic-features.rst
14:29:32 mriedem gibi: if you do that, anyone that doesn't have that code in their cloud can have users still attaching those kinds of ports
14:30:08 mriedem if we consider it a bug that we don't support ports with qos policies, then we should make that a hard failure (bug fix) and then add a new microversion which adds the support for those types of ports
14:31:08 gibi mriedem: so making it a hard failure is a bugfix, I like that
14:31:17 alex_xu_ emm...wait, we don't check the network has qos policy, we check the port has required resource or not. That is something new added in neutron
14:31:33 mriedem gibi: well that's what we did for the bfv rebuild case
14:32:30 gibi alex_xu_: required resources is just another representation of the QoS policy to let neutron do the transformation between QoS policy entity on the API to the resource classes and traits in the Placement
14:32:49 mriedem tbh, we're inconsistent on this type of thing. with the multiattach support in queens, we check if the volume is multiattach and if the microversion is high enough (2.60) and if not, we fail. but i'm pretty sure in pike you could probably attach a multiattach volume to at least one instance at a time without failures.
14:33:40 alex_xu_ gibi: the neutron side add 'resources' field to the port API by extension? or just add directly?
14:33:41 gibi cdent, edleafe, mriedem: I start to get convinced that making the API hardfail instead of accept and lie is a bugfix
14:35:06 mriedem so the bug is today we are potentially over-subscribing the qos bandwidth right?
14:35:10 mriedem for a given network
14:35:15 gibi alex_xu_: that will come from the QoS neutron plugin based on my current understanding
14:35:36 gibi alex_xu_: the neutron spec will describe this in detail (writing is in progress)
14:35:59 mriedem the neutron qos stuff has been around for a long time hasn't it? kind of surprised no one hasn't already reported this as a bug.
14:36:24 gibi mriedem: yes, SRIOV ports with QoS minimum bandwidth policies are not properly enforced and the minimum bandwidth cannot be garanteed
14:36:39 gibi mriedem: only SRIOV ports supporting minimum bandwidth policies
14:37:39 gibi mriedem: and there was a release notes that stated the problem
14:37:47 gibi mriedem: https://github.com/openstack/neutron/blob/49d614895f44c44f9e1735210498facf1886c404/releasenotes/notes/qos-min-egress-bw-rule-b1c80f5675a4c1c3.yaml
14:38:33 gibi mriedem: so maybe the deployers read the release notes and understood that the support for the minimum bandwidth rule is incomplete
14:38:49 mriedem added in newton
14:38:53 gibi mriedem: yes
14:39:30 mriedem looks like the policy defaults allow only admins to create these types of policies
14:39:33 mriedem so there is at least that
14:42:42 openstackgerrit Surya Seetharaman proposed openstack/nova master: Add disabled field to CellMapping object https://review.openstack.org/550090
14:42:43 openstackgerrit Surya Seetharaman proposed openstack/nova master: Add CellMappingList.get_all_enabled() query method https://review.openstack.org/550188
14:42:43 openstackgerrit Surya Seetharaman proposed openstack/nova master: Allow scheduling only to enabled cells (Filter Scheduler) https://review.openstack.org/550527
14:42:49 bauzas sean-k-mooney[m]: jaypipes: stephenfin: soft -1 on https://review.openstack.org/#/c/545951/2 but I need to understand why we can't just get the port info in the conductor, and pass the traits to the scheduler
14:43:36 bauzas I could be wrong but AFAIK we haven't said why it wasn't possible to do the above ^
14:57:02 dansmith jaypipes: sean-k-mooney[m]: do either of you know if/when we can remove the old vif plugging stuff from the libvirt driver? i.e. everything reachable past the "if not os-vif" line?
14:57:37 openstackgerrit Ed Leafe proposed openstack/nova master: Address issues raised in adding member_of to GET /a-c https://review.openstack.org/554357
14:57:39 edleafe efried: ^^ yay pep8!
15:14:48 mriedem dansmith: presumably once all of the legacy methods in libvirt/vif.py are converted to using os-vif objects
15:15:21 dansmith mriedem: I don't know how to tell that. are those just linux bridge and ovs right now?
15:16:03 mriedem look in nova.network.os_vif_util at the _nova_to_osvif_vif_* methods that raise NotImplementedError
15:16:06 mriedem there are quite a few
15:16:23 stephenfin dansmith: Yeah, once all of those have been converted
15:17:09 stephenfin jaypipes and I have discussed it before. The IVS driver has been converted to an os-vif plugin but I don't know about the rest of them. We might need to jettison them or bring them into os-vif core

Earlier   Later