[ovs-dev] [PATCH v4 1/3] Implement Openflow 1.4 Vacancy Events for OFPT_TABLE_MOD.

[Saloni Jain]

Hi Ben,

Thanks for the review. I will do all the suggested changes in v5 of the patch series.

Please validate my understanding for the following 2 comments:

>In ovs-ofctl.c, I don't think the table-mod code handles the case where
>OF1.4 or OF1.5 is enabled but the switch does not support it.

This means that in ovs-ofctl.c, for table-mod, it can happen that OF1.4 and OF1.5 are supported, but the switch in table-features capabilities does not support eviction or vacancy table-config parameters.
So after checking the usable version as OF1.4 and OF1.5 for table-mod in ovs-ofctl.c, table feature request should be sent to the switch from ofctl_mod_table() in order to get the supported capabilities for the given table-id and if eviction/vacancy events are supported by switch, then only table-mod config property should be set.


>The syntax seems kind of odd actually.  How about "vacancy(low,high)"?

Parentheses - "()" and "{}" are used for command grouping in shell and will give error "syntax error:bash: syntax error near unexpected token `('", when used with any command.
So in order to avoid the error, we have to use escape characters or single/double quotes around "vacancy(low,high)" in ovs-ofctl mod-table command, such that, the command looks like:
                     ovs-ofctl -O Openflow14 mod-table br0 0 'vacancy(low,high)'

I have also tried for "vacancy[low,high]", that is using square brackets [], but I am facing problem in test cases. In file ofproto.at square brackets are ignored in AT_CHECK[] and so test case for mod-table for vacancy is failing.
Other possible syntax are -- vacancy:low-high or vacancy:low,high

Please suggest whether to go with parentheses "vacancy(low,high)" or is there any other recommended syntax which can be used? 

Thanks and Regards,
Saloni Jain
Tata Consultancy Services
Mailto: saloni.jain at tcs.com
Website: http://www.tcs.com
____________________________________________
Experience certainty.	IT Services
Business Solutions
Consulting
____________________________________________


-----"dev" <dev-bounces at openvswitch.org> wrote: -----
To: saloni.jain12 at gmail.com
From: Ben Pfaff 
Sent by: "dev" 
Date: 09/18/2015 04:00AM
Cc: dev at openvswitch.org, Shashwat Srivastava <shashwat.srivastava at tcs.com>, deepankar.gupta at tcs.com, partha.datta at tcs.com, Sandeep Kumar <sandeep.kumar16 at tcs.com>
Subject: Re: [ovs-dev] [PATCH v4 1/3] Implement Openflow 1.4 Vacancy Events for OFPT_TABLE_MOD.

On Wed, Sep 09, 2015 at 01:12:04PM +0530, saloni.jain12 at gmail.com wrote:
> From: Saloni Jain <saloni.jain at tcs.com>
> 
> OpenFlow 1.4 introduces the ability to turn on vacancy events with an
> OFPT_TABLE_MOD message specifying OFPTC_VACANCY_EVENTS. This commit adds
> support for the new feature in ovs-ofctl mod-table.
> As per the openflow specification-1.4, vacancy event adds a mechanism
> enabling the controller to get an early warning based on capacity
> threshold chosen by the controller.
> 
> With this commit, vacancy events can be configured as:
> ovs-ofctl -O OpenFlow14 mod-table <bridge> <table> vacancy-<range>
> The syntax of <range> as <low..high>.
> <range> specify vacancy threshold values, vacancy down and vacancy up.
> 
> To disable vacancy events, following command should be given:
> ovs-ofctl -O OpenFlow14 mod-table <bridge> <table> novacancy
> 
> Signed-off-by: Saloni Jain <saloni.jain at tcs.com>
> Co-authored-by: Shashwat Srivastava <shashwat.srivastava at tcs.com>
> Signed-off-by: Shashwat Srivastava <shashwat.srivastava at tcs.com>
> Co-authored-by: Sandeep Kumar <sandeep.kumar16 at tcs.com>
> Signed-off-by: Sandeep Kumar <sandeep.kumar16 at tcs.com>

Thanks for the patch!

This needs to update the documentation for ovs-ofctl to mention the new
feature and describe the syntax.

The syntax seems kind of odd actually.  How about "vacancy(low,high)"?

I think that the format output by ofp_print_table_mod() should match
that accepted by ofp-parse instead of being completely different.

One of the patches in this series should update NEWS to mention the new
feature.

The 'vacancy' member in struct oftable appears to have exactly two
values: 0 and OFPTC14_VACANCY_EVENTS.  Why is it an "unsigned int"
instead of a Boolean (e.g. "bool vacancy_enabled")?

I think that parse_table_mod_vacancy_property() should report an error
for a "percentage" that exceeds 100.

In a table_mod the 'vacancy' member of struct
ofp14_table_mod_prop_vacancy is supposed to be always zero, so I don't
see why ofputil_encode_table_mod() copies this in from its argument
instead of leaving it zeroed.

In ovs-ofctl.c, I don't think the table-mod code handles the case where
OF1.4 or OF1.5 is enabled but the switch does not support it.

Thanks,

Ben.
_______________________________________________
dev mailing list
dev at openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
=====-----=====-----=====
Notice: The information contained in this e-mail
message and/or attachments to it may contain 
confidential or privileged information. If you are 
not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the 
information contained in this e-mail message 
and/or attachments to it are strictly prohibited. If 
you have received this communication in error, 
please notify us by reply e-mail or telephone and 
immediately and permanently delete the message 
and any attachments. Thank you