| Message ID | 20191223103628.20787-1-taoyunxiang@cmss.chinamobile.com |
|---|---|
| Headers | show |
| Series | Add a way to delete QoS directly | expand |
On Mon, Dec 23, 2019 at 2:37 AM Yunxiang Tao < taoyunxiang@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?
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.
2. As for point 2, I am agreed, but do not know how to do now.
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.
[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 <taoyunxiang@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?
Hi Han,
If you have time, Could you review this patch or give a response.
Thanks ,
Yun
From: taoyunxiang@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.
2. As for point 2, I am agreed, but do not know how to do now.
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.
[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 <taoyunxiang@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?
Sorry for the late reply. Please see my response below. On Wed, Jan 8, 2020 at 5:16 PM taoyunxiang@cmss.chinamobile.com < taoyunxiang@cmss.chinamobile.com> wrote: > > Hi Han, > If you have time, Could you review this patch or give a response. > > > Thanks , > Yun > > > > From: taoyunxiang@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 < taoyunxiang@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? >
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.
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.
2. Alter the methods which has used "name" to record the mappings .
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: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com <taoyunxiang@cmss.chinamobile.com> wrote:
>
> Hi Han,
> If you have time, Could you review this patch or give a response.
>
>
> Thanks ,
> Yun
>
>
>
> From: taoyunxiang@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 <taoyunxiang@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?
>
Hi Yun, On Fri, Jan 10, 2020 at 3:10 AM taoyunxiang@cmss.chinamobile.com < taoyunxiang@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: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com < taoyunxiang@cmss.chinamobile.com> wrote: > > > > Hi Han, > > If you have time, Could you review this patch or give a response. > > > > > > Thanks , > > Yun > > > > > > > > From: taoyunxiang@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 < taoyunxiang@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? > >
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: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com <taoyunxiang@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: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com <taoyunxiang@cmss.chinamobile.com> wrote:
> >
> > Hi Han,
> > If you have time, Could you review this patch or give a response.
> >
> >
> > Thanks ,
> > Yun
> >
> >
> >
> > From: taoyunxiang@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 <taoyunxiang@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?
> >
On Tue, Jan 14, 2020 at 1:11 AM taoyunxiang@cmss.chinamobile.com < taoyunxiang@cmss.chinamobile.com> wrote: > > 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? You may start looking at lib/db-ctl-base.c which is used by all the ctl tools (e.g. ovs-vsctl, ovn-nbctl, ovn-sbctl) > Bye the way, how to reply email with automaticly incresing ">" or ">>.....". :):) Oh, this depends on your email client. Gmail does this when replying with text mode :) > > Thanks, > YUN > ________________________________ > > > From: Han Zhou > Date: 2020-01-11 01:54 > To: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com < taoyunxiang@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: taoyunxiang@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 taoyunxiang@cmss.chinamobile.com < taoyunxiang@cmss.chinamobile.com> wrote: > > > > > > Hi Han, > > > If you have time, Could you review this patch or give a response. > > > > > > > > > Thanks , > > > Yun > > > > > > > > > > > > From: taoyunxiang@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 < taoyunxiang@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? > > >