Hi Han, Thank you for your patience in answering my questions. I agree with your comments. I am interested in "specifying uuid when creating a row is not yet supported by the ctl tools", I will try to summit patch to support it . Do you have some sugesstion? Bye the way, how to reply email with automaticly incresing ">" or ">>.....". :):) Thanks, YUN
From: Han Zhou Date: 2020-01-11 01:54 To: taoyunxi...@cmss.chinamobile.com CC: Han Zhou; ovs-dev Subject: Re: Re: [ovs-dev] [PATCH ovn v4 0/2] Add a way to delete QoS directly Hi Yun, On Fri, Jan 10, 2020 at 3:10 AM taoyunxi...@cmss.chinamobile.com <taoyunxi...@cmss.chinamobile.com> wrote: > > Hi Han, > Thanks for your reply. I agree with your following comment 1. For > the comment 2, I think Ben' s patch will make "UUID" and "name" be similar, I > don't know the reason for summit this patch, I have comment for that patch in > another email. > Ben's patch "Allow OVSDB clients to specify the UUID for inserted rows" was initially intended for DDlog's use case to reduce memory footprint in DDlog. However, I think it is very useful for many other OVSDB clients, such as networking-ovn. With this feature, clients doesn't need to store a mapping between its own objects and the rows in OVSDB, either by storing the its own ID in external-ids, or by saving the mapping in its own DB. Instead, clients can generate the object uuid and tell OVSDB to use the same uuid as the row uuid. I feel it is also the feature you are looking for to solve the efficiency of QoS operations. I agree that you will need some work in networking-ovn to benefit from this feature. > Enventually, If we use Ben's patch to record mappings between OVN > and Neutron,we may also need to finish two jobs: > 1. A patch to make QoS could be search by UUID, which not > supported now, has finished in my patch 2. For read/update/delete operations, OVSDB already supports directly using uuid to identify a row, and all the ovs/ovn-xxctl tools supports this (see man page of any ctl tool, e.g. man ovn-nbctl, in section "Database Commands"). So even without any change, users are able to operating any records using uuid. However, I agree that it is more user-friendly if we add the support of operations on uuid to the QoS related commands. And also, specifying uuid when creating a row is not yet supported by the ctl tools (I have mentioned it in the code review, and I think it should be an easy task to add it). > 2. Alter the methods which has used "name" to record the > mappings . Do you mean in OVN or in Neutron networking-ovn? In general I don't think we need to alter any existing methods, because they will just continue to work. I agree that we will need some work in networking-ovn to benefit from this feature, as an optimization, such as for QoS operations. And eventually we may convert other operations in networking-ovn to benefit from this feature, if needed. Before this feature can be used by networking-ovn, probably the first thing to do is to update the python ovsdbapp to support specifying uuid when creating a record. > > What do you think? :) > > Thanks, > Yun > > > I agree with you that it is more efficient to delete with an ID > instead of searching based on fields. I think we need to consider below two > factors: > > 1. If the new "name" column is supposed to uniquely identify a > row, it is better to add the "index" constraint in the schema for this new > column, to avoid duplicated names. > > 2. Ben recently worked on the new feature of OVSDB to make it > allows client to specify row uuid when creating a row [0]. If this patch is > merged, we don't need a "name" column to uniquely identify a row any more. > I'd > prefer to utilize this new feature and to avoid > adding new "name" columns for individual tables. Do you think this is helpful? > > [0] https://patchwork.ozlabs.org/patch/1220644/ > > > From: Han Zhou > Date: 2020-01-10 12:03 > To: taoyunxi...@cmss.chinamobile.com > CC: ovs-dev > Subject: Re: Re: [ovs-dev] [PATCH ovn v4 0/2] Add a way to delete QoS directly > Sorry for the late reply. Please see my response below. > > On Wed, Jan 8, 2020 at 5:16 PM taoyunxi...@cmss.chinamobile.com > <taoyunxi...@cmss.chinamobile.com> wrote: > > > > Hi Han, > > If you have time, Could you review this patch or give a > > response. > > > > > > Thanks , > > Yun > > > > > > > > From: taoyunxi...@cmss.chinamobile.com > > Date: 2019-12-24 19:55 > > To: Han Zhou > > CC: ovs-dev > > Subject: Re: Re: [ovs-dev] [PATCH ovn v4 0/2] Add a way to delete QoS > > directly > > Hi Han, > > Thanks for your review! > > > > 1. As for point 1 and point 4, I think it is not bad to use > > external_ids to record association between Neutron and OVN, even we can > > record LS info also. But it is not directly and clearly. > > Actually, I find almost resource tables have "name" column, > > included ACL table, which summited by this patch[0]. The purpose of adding > > "name" for ACL, is also to make ACL rule could be easily find and check in > > OVN. > > Ok, the name column of ACL table was added for the logging purpose, but I > agree with you it can be used the way you intended to. > > > > > 2. As for point 2, I am agreed, but do not know how to do now. > > There are several ways. > Option1: Keep the old command and add a new command for deleting by name. > Option2: Add an option to the old command such as "--by-name" to delete by > name. > Option3: Keep the SWITCH arg when deleting by name, but only for the given > switch. If the next argument can be found as a QoS name in the switch, it is > treated as delete by name, otherwise, it is handled as before. > > > > > 3. As for point 3, I think we can record LS info, it is not bad. > > > > > > As for why I want to change "qos-del" comand, I want to explain more > > detailed. Hope to get your advice. > > > > 1. QoS rule resource of Neutron is not mapping any resource of OVN, and > > it just contains rate, burst, dircetion and so on. > > > > 2. In Neutron, when we bind QoS rule to port or network, it will triger > > OVN to record in QoS table. > > > > 3. When we update QoS rule in Neutron , it will DIRECTLY update its > > record of Neutron DB(will not wait OVN to update, because no mapping > > resource), gradually, it will update port or network which binded this QoS > > rule. But the QoS rule has already become the lasted one in Neutron DB, and > > networking-ovn can not get the old QoS rule of Neutron. So we can not > > update it exactly. > > > > 4. For the above situaiton, we can only delete old rules that have the > > same direction as the new rules, or , we will expand the scope of deletion. > > This patch [1] is the logic process for networking-ovn to execute > > QoS rule. The update process is not exactly and suitable, I think the Core > > OVN may need some changes. > > > > I agree with you that it is more efficient to delete with an ID instead of > searching based on fields. I think we need to consider below two factors: > 1. If the new "name" column is supposed to uniquely identify a row, it is > better to add the "index" constraint in the schema for this new column, to > avoid duplicated names. > 2. Ben recently worked on the new feature of OVSDB to make it allows client > to specify row uuid when creating a row [0]. If this patch is merged, we > don't need a "name" column to uniquely identify a row any more. I'd prefer to > utilize this new feature and to avoid adding new "name" columns for > individual tables. Do you think this is helpful? > > [0] https://patchwork.ozlabs.org/patch/1220644/ > > > > > [0] > > https://github.com/ovn-org/ovn/commit/17df18dac9d2fa875c2f86a54bbc84931689d42c > > [1] https://review.opendev.org/#/c/692084/ > > > > > > Hope to have your advice. > > > > > > Thanks, > > Yun > > > > > > From: Han Zhou > > Date: 2019-12-24 01:33 > > To: Yunxiang Tao > > CC: ovs-dev > > Subject: Re: [ovs-dev] [PATCH ovn v4 0/2] Add a way to delete QoS directly > > > > > > On Mon, Dec 23, 2019 at 2:37 AM Yunxiang Tao > > <taoyunxi...@cmss.chinamobile.com> wrote: > > > > > > Currently, qos can only be deleted by indirect way which must designate > > > switch or more parameters. The first patch add "name" column to QoS table, > > > and make QoS table could be listed by "name" witch comand > > > "ovn-nbctl list qos "name"". > > > > > > The second patch change the original "qos-del" to "ls-qos-del", add > > > a new "qos-del" command. By the new command, you can delete qos > > > by uuid or name of the qos. It is useful. For example, we can associate > > > the qos table in neutron and OVN by "name" of qos in OVN, so neutron > > > could find and easily delete the corresponding qos in OVN. > > > > > > > Thanks for the patch. I have below comments: > > 1. There are other ways to associate QoS between Neutron and OVN. For > > example, the external-ids column can be used to add any customized > > key-value pairs, such as external_ids:neutron:qos_name = <neutron qos > > name>. Is this sufficient for your use case? > > 2. If we believe it is useful for qos-del command to delete qos by > > name/uuid, it is better to extend the exist command "qos-del" to handle > > both old and new formats, and we should NOT rename the existing command > > because it will break existing customer tools. > > 3. It is more efficient to specify the lswitch in the qos-del command so > > that it doesn't need to iterate every lswitch. Is there a use case that > > neutron needs to delete a QoS record without knowing which lswitch should > > it be deleted from? > > 4. QoS and ACL are implemented in similar way with similar principles. If > > "name" is needed in QoS table, probably it is also needed in ACL table. In > > other words, if we can handle ACL well without introducing the "name" > > column, probably we could do the same for QoS table without problem. What's > > your thought on this? > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev