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

Currently, rule_insert() API does not have return value. There are
some possible scenarios where rule insertions can fail at run-time
even though the static checks during rule_construct() had passed
previously.
Some possible scenarios for failure of rule insertions:
**) 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 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.
Rule-insert errors for bundles are handled too in this pull-request.

Testing:
-------
Tested failures of rule insertions and also with bundles.

Signed-off-by: Aravind Prasad S <aravind.sridharan@dell.com>

---
ofproto/ofproto-dpif.c     |   4 +-
ofproto/ofproto-provider.h |   6 +--
ofproto/ofproto.c          | 109 +++++++++++++++++++++++++++++++++------------
3 files changed, 86 insertions(+), 33 deletions(-)

static void ofproto_flow_mod_revert(struct ofproto *,
                                     struct ofproto_flow_mod *)
     OVS_REQUIRES(ofproto_mutex);
-static void ofproto_flow_mod_finish(struct ofproto *,
-                                    struct ofproto_flow_mod *,
-                                    const struct openflow_mod_requester *)
+static enum ofperr ofproto_flow_mod_finish(struct ofproto *,
+                                           struct ofproto_flow_mod *,
+                                           const struct openflow_mod_requester *)
     OVS_REQUIRES(ofproto_mutex);
static enum ofperr handle_flow_mod__(struct ofproto *,
                                      const struct ofputil_flow_mod *,
@@ -5115,7 +5115,7 @@ add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
}
 /* To be called after version bump. */
-static void
+static enum ofperr
add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                 const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
@@ -5124,8 +5124,14 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         ? rule_collection_rules(&ofm->old_rules)[0] : NULL;
     struct rule *new_rule = rule_collection_rules(&ofm->new_rules)[0];
     struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
+    enum ofperr error = 0;
+
+    error = replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
+                                &dead_cookies);
+    if (error) {
+        return error;
+    }
-    replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, &dead_cookies);
     learned_cookies_flush(ofproto, &dead_cookies);
     if (old_rule) {
@@ -5138,6 +5144,8 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         /* Send Vacancy Events for OF1.4+. */
         send_table_status(ofproto, new_rule->table_id);
     }
+
+    return error;
}

 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
@@ -5334,22 +5342,25 @@ ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm)
     ofproto_flow_mod_revert(rule->ofproto, ofm);
}
-void
+enum ofperr
ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
                               struct ofproto *orig_ofproto)
     OVS_REQUIRES(ofproto_mutex)
{
     struct rule *rule = rule_collection_rules(&ofm->new_rules)[0];
+    enum ofperr error = 0;
     /* If learning on a different bridge, must bump its version
      * number and flush connmgr afterwards. */
     if (rule->ofproto != orig_ofproto) {
         ofproto_bump_tables_version(rule->ofproto);
     }
-    ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
+    error = ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
     if (rule->ofproto != orig_ofproto) {
         ofmonitor_flush(rule->ofproto->connmgr);
     }
+
+    return error;
}
 /* Refresh 'ofm->temp_rule', for which the caller holds a reference, if already
@@ -5404,7 +5415,7 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref,
             error = ofproto_flow_mod_learn_start(ofm);
             if (!error) {
-                ofproto_flow_mod_learn_finish(ofm, NULL);
+                error = ofproto_flow_mod_learn_finish(ofm, NULL);
             }
         } else {
             static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -5504,7 +5515,7 @@ replace_rule_revert(struct ofproto *ofproto,
}
 /* Adds the 'new_rule', replacing the 'old_rule'. */
-static void
+static enum ofperr
replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                     const struct openflow_mod_requester *req,
                     struct rule *old_rule, struct rule *new_rule,
@@ -5512,6 +5523,8 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     OVS_REQUIRES(ofproto_mutex)
{
     struct rule *replaced_rule;
+    enum ofperr error = 0;
+    struct oftable *table = &ofproto->tables[new_rule->table_id];
     replaced_rule = (old_rule && old_rule->removed_reason != OFPRR_EVICTION)
         ? old_rule : NULL;
@@ -5521,8 +5534,15 @@ 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) {
+        if (classifier_remove(&table->cls, &new_rule->cr)) {
+            ofproto_rule_destroy__(new_rule);
+        }
+        return error;
+    }
+
     learned_cookies_inc(ofproto, rule_get_actions(new_rule));
     if (old_rule) {
@@ -5558,6 +5578,8 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                              req ? req->request->xid : 0, NULL);
         }
     }
+
+    return error;
}
 /* ofm->temp_rule is consumed only in the successful case. */
@@ -5706,17 +5728,18 @@ modify_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     }
}
-static void
+static enum ofperr
modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                     const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
{
     struct rule_collection *old_rules = &ofm->old_rules;
     struct rule_collection *new_rules = &ofm->new_rules;
+    enum ofperr error = 0;
     if (rule_collection_n(old_rules) == 0
         && rule_collection_n(new_rules) == 1) {
-        add_flow_finish(ofproto, ofm, req);
+        error = add_flow_finish(ofproto, ofm, req);
     } else if (rule_collection_n(old_rules) > 0) {
         struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
@@ -5725,12 +5748,17 @@ modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         struct rule *old_rule, *new_rule;
         RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) {
-            replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
-                                &dead_cookies);
+            error = replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
+                                        &dead_cookies);
+            if (error) {
+                return error;
+            }
         }
         learned_cookies_flush(ofproto, &dead_cookies);
         remove_rules_postponed(old_rules);
     }
+
+    return error;
}
 static enum ofperr
@@ -6094,7 +6122,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
     error = ofproto_flow_mod_start(ofproto, &ofm);
     if (!error) {
         ofproto_bump_tables_version(ofproto);
-        ofproto_flow_mod_finish(ofproto, &ofm, req);
+        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
         ofmonitor_flush(ofproto->connmgr);
     }
     ovs_mutex_unlock(&ofproto_mutex);
@@ -7960,19 +7988,21 @@ ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     rule_collection_destroy(&ofm->new_rules);
}
-static void
+static enum ofperr
ofproto_flow_mod_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                         const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
{
+    enum ofperr error = 0;
+
     switch (ofm->command) {
     case OFPFC_ADD:
-        add_flow_finish(ofproto, ofm, req);
+        error = add_flow_finish(ofproto, ofm, req);
         break;
     case OFPFC_MODIFY:
     case OFPFC_MODIFY_STRICT:
-        modify_flows_finish(ofproto, ofm, req);
+        error = modify_flows_finish(ofproto, ofm, req);
         break;
     case OFPFC_DELETE:
@@ -7990,6 +8020,8 @@ ofproto_flow_mod_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     if (req) {
         ofconn_report_flow_mod(req->ofconn, ofm->command);
     }
+
+    return error;
}
 /* Commit phases (all while locking ofproto_mutex):
@@ -8073,11 +8105,9 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         if (error) {
             /* Send error referring to the original message. */
-            if (error) {
-                ofconn_send_error(ofconn, be->msg, error);
-                error = OFPERR_OFPBFC_MSG_FAILED;
-            }
-
+            ofconn_send_error(ofconn, be->msg, error);
+            error = OFPERR_OFPBFC_MSG_FAILED;
+
             /* 2. Revert.  Undo all the changes made above. */
             LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
                 if (be->type == OFPTYPE_FLOW_MOD) {
@@ -8119,13 +8149,34 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                     struct openflow_mod_requester req = { ofconn, be->msg };
                     if (be->type == OFPTYPE_FLOW_MOD) {
-                        ofproto_flow_mod_finish(ofproto, &be->ofm, &req);
+                        error = ofproto_flow_mod_finish(ofproto, &be->ofm,
+                                                        &req);
                     } else if (be->type == OFPTYPE_GROUP_MOD) {
                         ofproto_group_mod_finish(ofproto, &be->ogm, &req);
                     } else if (be->type == OFPTYPE_PACKET_OUT) {
                         ofproto_packet_out_finish(ofproto, &be->opo);
                     }
                 }
+                if (error) {
+                    break;
+                }
+            }
+            if (error) {
+                /* Send error referring to the original message. */
+                ofconn_send_error(ofconn, be->msg, error);
+                error = OFPERR_OFPBFC_MSG_FAILED;
+
+                /* Revert.  Undo all the changes made above. */
+                LIST_FOR_EACH_REVERSE_CONTINUE (be, node, &bundle->msg_list) {
+                    if (be->type == OFPTYPE_FLOW_MOD) {
+                        ofproto_flow_mod_revert(ofproto, &be->ofm);
+                    } else if (be->type == OFPTYPE_GROUP_MOD) {
+                        ofproto_group_mod_revert(ofproto, &be->ogm);
+                    } else if (be->type == OFPTYPE_PACKET_OUT) {
+                        ofproto_packet_out_revert(ofproto, &be->opo);
+                    }
+                    /* Nothing needs to be reverted for a port mod. */
+                }
             }
         }
--
2.7.4

Bleep bloop.  Greetings , I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: patch fragment without header at line 19: @@ -5115,7 +5115,7 @@ add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 ofproto: Return error codes for Rule insertions
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

On Tue, Apr 23, 2019 at 02:40:55PM +0000, Aravind.Sridharan@dell.com wrote:
> Currently, rule_insert() API does not have return value. There are
> some possible scenarios where rule insertions can fail at run-time
> even though the static checks during rule_construct() had passed
> previously.
> Some possible scenarios for failure of rule insertions:
> **) 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 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.
> Rule-insert errors for bundles are handled too in this pull-request.

It looks like you're not having much luck with posting patches.  If you
haven't already tried "git send-email", please try that; otherwise, you
can always submit a pull request on Github.

Thanks,

Ben.

Hi All,

I have raised the pull request in Github - https://github.com/openvswitch/ovs/pull/282. Kindly review and let me know your views. 

Hi Ben,
Also, I raised the similar patch sometime back and you had requested for the vendor name. Had to get approval from my Organization. Hence, requesting your review for the changes now. 

> Hi Ben,
> 
> By "which" switches, you mean to ask to which vendor switches ?
> In general, this patch will be very useful for all Switches which
> implement OVS for their HW implementations, specifically,
> will be useful for Switches which had implemented ofproto
> and netdev provider APIs for their respective HW/NPU and
> Kernel. And, in case of HW, the possibility of dynamic rule
> insertion failures are usually higher. Static checks during
> rule-construct phase are not sufficient and predicting
> future failure predictions in construct phase may not be possible.
> Hence, this patch will make OVS more flexible to handle such
> rule insertion failures.

> So, basically, you're not willing to name any switches.  I'm tired of
> the vendor secrecy game.  They get to see all of our code but they're
>never willing even to name themselves.

> Sorry, I'm not going to willingly take this patch.


Thanks,
S. Aravind Prasad

On Tue, Apr 23, 2019 at 02:40:55PM +0000, Aravind.Sridharan@dell.com wrote:
> Currently, rule_insert() API does not have return value. There are 
> some possible scenarios where rule insertions can fail at run-time 
> even though the static checks during rule_construct() had passed 
> previously.
> Some possible scenarios for failure of rule insertions:
> **) 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 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.
> Rule-insert errors for bundles are handled too in this pull-request.

> It looks like you're not having much luck with posting patches.  If you  > haven't already tried "git send-email", please try that; otherwise, you > can always submit a pull request on Github.

Thanks,

Ben.

Hi All,

Sorry for resending the mail, the link appears wrongly in previous mail (due to mingling of 'dot' with github link). 

Correct GitHub pull request link - https://github.com/openvswitch/ovs/pull/282   

Kindly review.

Thanks,
Aravind Prasad S

-----Original Message-----
From: Sridharan, Aravind 
Sent: Wednesday, April 24, 2019 1:26 AM
To: 'Ben Pfaff'; ovs-dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions

Hi All,

I have raised the pull request in Github - https://github.com/openvswitch/ovs/pull/282. Kindly review and let me know your views. 

Hi Ben,
Also, I raised the similar patch sometime back and you had requested for the vendor name. Had to get approval from my Organization. Hence, requesting your review for the changes now. 

> Hi Ben,
> 
> By "which" switches, you mean to ask to which vendor switches ?
> In general, this patch will be very useful for all Switches which 
> implement OVS for their HW implementations, specifically, will be 
> useful for Switches which had implemented ofproto and netdev provider 
> APIs for their respective HW/NPU and Kernel. And, in case of HW, the 
> possibility of dynamic rule insertion failures are usually higher. 
> Static checks during rule-construct phase are not sufficient and 
> predicting future failure predictions in construct phase may not be 
> possible.
> Hence, this patch will make OVS more flexible to handle such rule 
> insertion failures.

> So, basically, you're not willing to name any switches.  I'm tired of  
>the vendor secrecy game.  They get to see all of our code but they're 
>never willing even to name themselves.

> Sorry, I'm not going to willingly take this patch.


Thanks,
S. Aravind Prasad

On Tue, Apr 23, 2019 at 02:40:55PM +0000, Aravind.Sridharan@dell.com wrote:
> Currently, rule_insert() API does not have return value. There are 
> some possible scenarios where rule insertions can fail at run-time 
> even though the static checks during rule_construct() had passed 
> previously.
> Some possible scenarios for failure of rule insertions:
> **) 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 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.
> Rule-insert errors for bundles are handled too in this pull-request.

> It looks like you're not having much luck with posting patches.  If you  > haven't already tried "git send-email", please try that; otherwise, you > can always submit a pull request on Github.

Thanks,

Ben.