[ovs-dev] ofproto: Return error codes for Rule insertions/deletions

Currently, rule_insert() and rule_delete() ofproto provider APIs do not


have return values. There are some possible scenarios where rule insertions

and deletions can fail at run-time even though the static checks during

rule_construct() had passed previously.



Some possible scenarios for failure of rule insertions and deletions:



**) Rule insertions can fail dynamically in Hybrid mode (both Openflow and


Normal switch functioning coexist) where the CAM space could get suddenly


filled up by Normal switch functioning and Openflow gets devoid of


available space.



**) Some deployments could have separate independent layers for HW rule


insertions/deletions and application layer to interact with OVS. HW layer


could face any dynamic issue during rule handling which application could


not have predicted/captured in rule-construction phase.





This patch is the first step to introduce error reporting for rule


insertions/deletions from Client back to OVS.


Signed-off-by: Aravind Prasad S <raja.avi@gmail.com>

---

         /* This may not be the last reference to the rule. */
@@ -2882,11 +2887,15 @@ remove_rule_rcu__(struct rule *rule)
 {
     struct ofproto *ofproto = rule->ofproto;
     struct oftable *table = &ofproto->tables[rule->table_id];
+    int error = 0;

     ovs_assert(!cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
     classifier_remove_assert(&table->cls, &rule->cr);
     if (ofproto->ofproto_class->rule_delete) {
-        ofproto->ofproto_class->rule_delete(rule);
+        error = ofproto->ofproto_class->rule_delete(rule);
+        if (error) {
+            VLOG_INFO("Rule deletion failed, error = %u", error);
+        }
     }
     ofproto_rule_unref(rule);
 }
@@ -5252,6 +5261,7 @@ replace_rule_finish(struct ofproto *ofproto,
struct ofproto_flow_mod *ofm,
     OVS_REQUIRES(ofproto_mutex)
 {
     struct rule *replaced_rule;
+    int error = 0;

     replaced_rule = (old_rule && old_rule->removed_reason != OFPRR_EVICTION)
         ? old_rule : NULL;
@@ -5261,8 +5271,12 @@ replace_rule_finish(struct ofproto *ofproto,
struct ofproto_flow_mod *ofm,
      * link the packet and byte counts from the old rule to the new one if
      * 'modify_keep_counts' is 'true'.  The 'replaced_rule' will be deleted
      * right after this call. */
-    ofproto->ofproto_class->rule_insert(new_rule, replaced_rule,
-                                        ofm->modify_keep_counts);
+    error = ofproto->ofproto_class->rule_insert(new_rule, replaced_rule,
+                                               ofm->modify_keep_counts);
+    if (error) {
+        VLOG_INFO("Rule insert failed, error=%u", error);
+    }
+
     learned_cookies_inc(ofproto, rule_get_actions(new_rule));

     if (old_rule) {

On Sat, Jun 16, 2018 at 06:14:38PM +0530, Aravind Prasad wrote:
> Currently, rule_insert() and rule_delete() ofproto provider APIs do not
> 
> 
> have return values. There are some possible scenarios where rule insertions
> 
> and deletions can fail at run-time even though the static checks during
> 
> rule_construct() had passed previously.
> 
> 
> 
> Some possible scenarios for failure of rule insertions and deletions:
> 
> 
> 
> **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
> 
> 
> Normal switch functioning coexist) where the CAM space could get suddenly
> 
> 
> filled up by Normal switch functioning and Openflow gets devoid of
> 
> 
> available space.
> 
> 
> 
> **) Some deployments could have separate independent layers for HW rule
> 
> 
> insertions/deletions and application layer to interact with OVS. HW layer
> 
> 
> could face any dynamic issue during rule handling which application could
> 
> 
> not have predicted/captured in rule-construction phase.
> 
> 
> 
> 
> 
> This patch is the first step to introduce error reporting for rule
> 
> 
> insertions/deletions from Client back to OVS.
> 
> 
> Signed-off-by: Aravind Prasad S <raja.avi@gmail.com>

Thanks for working to improve OVS.

Please don't triple-space your commit message.

This commit doesn't actually do anything to handle failures.  It only
reports them.  To be acceptable in OVS, it would have to actually handle
them.

(Also, for error reporting purposes, it's not acceptable to just print a
number.)

Thanks,

Ben.

> Currently, rule_insert() and rule_delete() ofproto provider APIs do not
>
>
> have return values. There are some possible scenarios where rule
insertions
>
> and deletions can fail at run-time even though the static checks during
>
> rule_construct() had passed previously.
>
>
>
> Some possible scenarios for failure of rule insertions and deletions:
>
>
>
> **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
>
>
> Normal switch functioning coexist) where the CAM space could get suddenly
>
>
> filled up by Normal switch functioning and Openflow gets devoid of
>
>
> available space.
>
>
>
> **) Some deployments could have separate independent layers for HW rule
>
>
> insertions/deletions and application layer to interact with OVS. HW layer
>
>
> could face any dynamic issue during rule handling which application could
>
>
> not have predicted/captured in rule-construction phase.
>
>
>
>
>
> This patch is the first step to introduce error reporting for rule
>
>
> insertions/deletions from Client back to OVS.
>
>
> Signed-off-by: Aravind Prasad S <raja.avi@gmail.com>

>>Thanks for working to improve OVS.

>>Please don't triple-space your commit message.

>>This commit doesn't actually do anything to handle failures.  It only
reports them.  To be acceptable in OVS, it would have to actually handle
them.

>>(Also, for error reporting purposes, it's not acceptable to just print a
number.)

Hi Ben,

Thanks for the review. Since the changes to delete the erroneous flow could
be a lot more, thought , it would be better to push the changes
incrementally.
Anyways, thanks again and will get back with the necessary changes.

Thanks,
S. Aravind Prasad

On Sat, Jun 16, 2018 at 9:19 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Sat, Jun 16, 2018 at 06:14:38PM +0530, Aravind Prasad wrote:
> > Currently, rule_insert() and rule_delete() ofproto provider APIs do not
> >
> >
> > have return values. There are some possible scenarios where rule
> insertions
> >
> > and deletions can fail at run-time even though the static checks during
> >
> > rule_construct() had passed previously.
> >
> >
> >
> > Some possible scenarios for failure of rule insertions and deletions:
> >
> >
> >
> > **) Rule insertions can fail dynamically in Hybrid mode (both Openflow
> and
> >
> >
> > Normal switch functioning coexist) where the CAM space could get suddenly
> >
> >
> > filled up by Normal switch functioning and Openflow gets devoid of
> >
> >
> > available space.
> >
> >
> >
> > **) Some deployments could have separate independent layers for HW rule
> >
> >
> > insertions/deletions and application layer to interact with OVS. HW layer
> >
> >
> > could face any dynamic issue during rule handling which application could
> >
> >
> > not have predicted/captured in rule-construction phase.
> >
> >
> >
> >
> >
> > This patch is the first step to introduce error reporting for rule
> >
> >
> > insertions/deletions from Client back to OVS.
> >
> >
> > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com>
>
> Thanks for working to improve OVS.
>
> Please don't triple-space your commit message.
>
> This commit doesn't actually do anything to handle failures.  It only
> reports them.  To be acceptable in OVS, it would have to actually handle
> them.
>
> (Also, for error reporting purposes, it's not acceptable to just print a
> number.)
>
> Thanks,
>
> Ben.
>