| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-10-10 | |||
| 13:35:30 | dansmith | jaypipes: _simply_ changing the master version shouldn't require a child bump, but there would be no reason | |
| 13:35:30 | jaypipes | dansmith: oh, am I confusing the composition rules with inheritance rules? | |
| 13:35:35 | stephenfin | sean-k-mooney: Nope. Rather, we just should make that if conditional on whether we're requesting SR-IOV VIFs or not | |
| 13:35:49 | jangutter | jaypipes, sean-k-mooney: I mean then you don't need to specifically associate _which_ VIF and which port-profile object/version combination go together, you just need to check if you understand each object/version combination separately. | |
| 13:35:55 | dansmith | jaypipes: I think you're confusing how including an object in another object used to require lockstep versioning, but that isn't the case any more | |
| 13:36:13 | sean-k-mooney | stephenfin: nope for macvtap i think you can set the model | |
| 13:36:22 | jaypipes | dansmith: right. I was confusing composition rules with inheritance rules. sorry (again) :( | |
| 13:36:25 | stephenfin | *SR-IOV direct | |
| 13:36:52 | sean-k-mooney | stephenfin: im also not sure if that assumtion is safe to make but it might be | |
| 13:37:10 | pooja_jadhav | mriedem: Let me try if u dont mind? | |
| 13:37:13 | stephenfin | moshele: Any thoughts on the above? | |
| 13:37:32 | stephenfin | moshele: tl;dr: I wonder if we should be setting 'model' here for SR-IOV VIFs? https://github.com/openstack/nova/blob/6bf11e1dc14afad78b11d980c2544a3dc41579ff/nova/virt/libvirt/vif.py#L130-L133 | |
| 13:37:42 | sean-k-mooney | dansmith: right but if the composed object version change how we make object compatible today is actully inccorect | |
| 13:37:46 | stephenfin | moshele: If we do and that hw_vif_model==virtio, we'll undo the hard work of https://github.com/openstack/nova/commit/622ebf2fab0a9bf75ee12437bef28f60e083f849 I think | |
| 13:38:02 | stephenfin | moshele: SR-IOV direct VIFs, that is | |
| 13:38:27 | jangutter | dansmith: if you use composition, would it make sense to check a list of classes and versions for compatibility, or would you need to check the combinations? | |
| 13:38:53 | sean-k-mooney | stephenfin: you can have VF that are virtio just an FYI | |
| 13:38:58 | dansmith | sean-k-mooney: I don't understand what you're saying | |
| 13:39:31 | dansmith | jangutter: heh, I'm also not sure what you mean | |
| 13:39:42 | dansmith | point me at code? | |
| 13:39:46 | jangutter | dansmith: let me set up a quick etherpad? | |
| 13:39:49 | moshele | stephenfin: sorry I don't follow | |
| 13:40:00 | dansmith | yah | |
| 13:40:05 | sean-k-mooney | dansmith: if you look at https://github.com/openstack/nova/blob/6bf11e1dc14afad78b11d980c2544a3dc41579ff/nova/objects/migrate_data.py#L243-L270 we call support with the target version fo the derived but that may not corralate to a target version of the base | |
| 13:40:12 | jaypipes | dansmith: they are wondering how the versioning system works when you have an ObjectField field... | |
| 13:40:30 | jaypipes | dansmith: and if the version of the ObjectField changes, why the base doesn't need to change. | |
| 13:40:53 | stephenfin | moshele: In https://github.com/openstack/nova/commit/622ebf2fab0a9bf75ee12437bef28f60e083f849 a check was added to ensure we don't see RX/TX queue sizes for non-virtio interfaces | |
| 13:41:08 | jangutter | jaypipes, sean-k-mooney, dansmith: https://etherpad.openstack.org/p/ovo-versioning | |
| 13:41:14 | dansmith | jaypipes: oh, because we send a manifest.. a list of object names and versions.. for rpc | |
| 13:41:23 | sean-k-mooney | jaypipes: yes. and in the past we used to have a verion map thing that track the version fo the object fileds and what version the correspondeed to | |
| 13:41:29 | dansmith | well, when we backport, we do that | |
| 13:41:59 | stephenfin | I'm thinking it's possible we could break the check if a user sets 'hw_vif_type=virtio' in image metadata. If they do that, then this line will be true | |
| 13:42:36 | stephenfin | https://github.com/openstack/nova/blob/6bf11e1dc14afad78b11d980c2544a3dc41579ff/nova/virt/libvirt/vif.py#L156 | |
| 13:43:01 | sean-k-mooney | dansmith: that is part of my argument as to why we should version for compoltion and inheritenc but since the has does not change for compostion versiosn its not enforece by our tests | |
| 13:43:05 | dansmith | sean-k-mooney: yeah that was a total disaster | |
| 13:43:14 | stephenfin | which wouldn't be the case normally because this line would be false for direct https://github.com/openstack/nova/blob/6bf11e1dc14afad78b11d980c2544a3dc41579ff/nova/virt/libvirt/vif.py#L137 | |
| 13:43:26 | dansmith | sean-k-mooney: it doesn't need to change | |
| 13:43:29 | sean-k-mooney | dansmith: the lockstep stuff. it was a pain but it worked | |
| 13:43:39 | dansmith | sean-k-mooney: it was unnecessary for us | |
| 13:43:49 | stephenfin | moshele: If that makes sense? I'm just thinking it's a latent bug. I'll test myself but doing so requires me setting up an environment :) | |
| 13:43:56 | dansmith | maybe for you because you don't have the same communication we have between nodes | |
| 13:44:48 | sean-k-mooney | dansmith: it maye if version 1 of my containing clase used version 1 of a composed calss and version 2 of the conatine uses version 5 then downgrading the containing class to 1 results in it having an object filed of 5 | |
| 13:44:52 | moshele | stephenfin: not sure, but you maybe right here | |
| 13:45:26 | dansmith | sean-k-mooney: yep, which is fine, because the objects are forward compatible | |
| 13:45:45 | mriedem | gibi: i'm late to this party but a few comments inline https://review.openstack.org/#/c/609330/ | |
| 13:45:47 | dansmith | sean-k-mooney: and if there's some reason, the containing class can downlevel the version it has | |
| 13:45:49 | mriedem | i haven't read the ML thread yet | |
| 13:46:00 | mriedem | pooja_jadhav: go ahead | |
| 13:46:53 | sean-k-mooney | dansmith: today we dont track that it should down level the object filed to 2 when it downlevels to 1 which is the gap | |
| 13:46:53 | moshele | stephenfin: please update me with your result | |
| 13:47:08 | stephenfin | moshele: Yup, will open a bug if it's an issue | |
| 13:47:42 | dansmith | sean-k-mooney: we don't backlevel it if and only if the requesting party says that they support version 5 of the sub object | |
| 13:47:43 | sean-k-mooney | dansmith: its totally solveable in the make_compatible function just we dont enforce it today and we should when we start passing os-vif objects over the api | |
| 13:47:50 | dansmith | sean-k-mooney: which wouldn't happen, and thus we'd backport both | |
| 13:47:53 | gibi | mriedem: you are not late at all | |
| 13:48:07 | dansmith | sean-k-mooney: sure we don't enforce it | |
| 13:48:10 | moshele | stephenfin: is 'hw_vif_type=virtio' in image metadata per network interface? | |
| 13:48:20 | dansmith | sean-k-mooney: that's why I'm saying if you are making objects where that matters, then you can backlevel it | |
| 13:48:25 | stephenfin | moshele: I don't think so. I don't see how it could be | |
| 13:48:40 | sean-k-mooney | dansmith: this isnt in relation to RPC by they way. this is relation to passing os-vif object betwen nova an neutron via the rest api | |
| 13:48:51 | stephenfin | moshele: Assuming you mean can it be configured per interface. I assume it affects all network interfaces | |
| 13:49:01 | moshele | stephenfin: I see, so we need to ignore it or reject it | |
| 13:49:06 | dansmith | sean-k-mooney: I know that, which is why I said you may have less flexibility because you don't control the communication in the same way | |
| 13:49:12 | stephenfin | For direct SR-IOV, I would imagine | |
| 13:49:32 | sean-k-mooney | dansmith: sure. we have added code in the past to do this and taken it out as part of code review because it did not go over rpc | |
| 13:49:34 | dansmith | sean-k-mooney: fwiw, the relation mapping stuff is all still in o.vo because it's a library, so you can use it if you really think it's necessary | |
| 13:49:46 | moshele | stephenfin: for VNIC_TYPES_DIRECT_PASSTHROUGH | |
| 13:49:52 | stephenfin | moshele: yeah | |
| 13:50:14 | sean-k-mooney | dansmith: ya we can but its also easy to do this in the make compatible fuction without that | |
| 13:50:40 | sean-k-mooney | jangutter: does any of this help? | |
| 13:51:10 | openstackgerrit | sean mooney proposed openstack/os-vif master: add support for generic tap device plug https://review.openstack.org/602384 | |
| 13:51:11 | openstackgerrit | sean mooney proposed openstack/os-vif master: clean up ip_command interface https://review.openstack.org/609414 | |
| 13:51:36 | jangutter | sean-k-mooney: I'm still downleveling the conversation to the simple version I can understand :-p | |
| 13:52:03 | gibi | mriedem: good point about the pre 2.29 and pre 2.30 microversion. I have to think that through. | |
| 13:52:41 | mriedem | gibi: i don't see operators on this thread either in the ML...has anyone reached out to the ops community to see if they are cool with dropping support for the force parameter? and if not, why not. | |
| 13:53:06 | gibi | mriedem: bauzas forwarded it to the ops I think | |
| 13:53:07 | mriedem | although as noted you can still use the force flag with the older microversion, or use 2.1 | |
| 13:53:11 | dansmith | jangutter: so you might be assuming there's more magic going on here than there is | |
| 13:53:19 | mriedem | this spec is really just about signaling | |
| 13:53:25 | mriedem | like when we deprecated personality files | |
| 13:53:25 | dansmith | jangutter: you can't define that a field is an object at (or below or above) a particular version | |
| 13:53:26 | bauzas | gibi: mriedem: yup but no answers yet | |
| 13:53:42 | bauzas | gibi: and I asked you about two separate microversions in the change | |
| 13:53:45 | jangutter | dansmith: right, this comes into play when serializing | |
| 13:53:55 | dansmith | jangutter: within a scope, it's assumed that you support a set of object schemas (by name) at a particular version, with compatibility for older versions | |
| 13:53:57 | mriedem | bauzas: no one but you wants 2 microversions for this | |
| 13:54:07 | gibi | bauzas: it seems to me that others more like having a single microversion instead | |
| 13:54:08 | mriedem | 2 microversions for the same thing is excessive | |
| 13:54:36 | mriedem | as efried noted in the spec review, we don't do multiple microversions for something just because it touches multiple APIs | |
| 13:54:38 | dansmith | jaypipes: so when we serialize today, we have the object and a list of versions that the other side supports, and we backlevel everything in the tree to the versions they support, as needed | |
| 13:54:41 | dansmith | er jangutter ^ | |
| 13:54:47 | mriedem | otherwise i would have had multiple microversions for volume multiattach and removing personality files | |
| 13:55:07 | dansmith | jangutter: if you don't have that manifest of supported versions, then you'd have to make inferences based on the version of the parent you're pinned to, which is what we used to do | |
| 13:55:40 | dansmith | jangutter: like this: https://github.com/openstack/nova/blob/kilo-eol/nova/objects/instance.py#L257 | |
| 13:55:51 | dansmith | jangutter: if you set those up on your object then o.vo will still respect them | |
| 13:55:58 | jangutter | dansmith: yep. the trick comes when downlevelling an object that has members containing other objects which are versioned. | |
| 13:56:15 | dansmith | jangutter: right, but that's what this obj_relationships is for | |
| 13:56:17 | sean-k-mooney | dansmith: that is assume that the latest version of each object both side knows about is the desired version correct | |
| 13:56:38 | dansmith | jangutter: note that it specifies which versions the contains-another-object fields refer to | |
| 13:56:44 | bauzas | mriedem: gibi: okay, just the fact that we had 2 different versions previously (2.29 and 2.30) | |