Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
On 9/11/20 11:13 AM, Vishal Deep Ajmera wrote: >> Hi, Vishal. I'm looking through old patches after the patchwork cleanup and >> it seems that this one in kind of undecided state. >> Does this patch still valid? >> >> Anyway, for the problem you described here: Maybe it's possible to just use >> 'ofport_request' configuration option for interfaces? It could be >> implemented >> as picking a unique ofport before adding a new port and set 'ofport_request' >> along with port creation. Another way is to fix current ofport for >> interfaces >> once OVS chose it, i.e. port add -> check ofport -> set 'ofport_request' >> with >> that value. >> In any case after reboot OVS will choose exactly same ofport as it was >> before >> because it is stored in database. >> >> What do you think? > > Hi Ilya, > > I agree that using ofp_request at the time of port creation will fix this > issue. > However this means an orchestrator also need to manage unique port > numbers for ports it is creating on OVS. I am not sure if this is feasible > for > all. This solution should not be much harder than below one. The entity that adds the port will just need to list port numbers of existing ports and peak any that is not used yet. > > Also on the other hand if we overload the field ofp_request and set it to ofp > number > automatically assigned by OVS, will it be misleading? However I think this > approach is cleaner. To clarify, I'm talking about ofport_request column in the interface table. What I'm proposing is that entity that executes port-add command, will later check the ofport field in the ovsdb and set this value to ofport_request column for this interface. Should not be misleading as all operations performed by the orchestrator, but instead of choosing unused value by itself it will rely on OVS to to do that. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
>> >> I've been taking a look at this patch for the last few minutes. It >> introduces a lot >> of mechanism for the use case. Did you consider any simpler mechanisms to >> achieve the same effect? What prevented them from working? >> > I agree Ben. This change does bring some complexity to the code. A simpler > solution would > be to leave the ofport number in ovsdb record of the interface (instead of > setting it to -1). > But issue was when the port delete happens we do not see this record to be > able to free the > ofport number. Is there a way to handle this? > > Warm Regards, > Vishal Ajmera Hi, Vishal. I'm looking through old patches after the patchwork cleanup and it seems that this one in kind of undecided state. Does this patch still valid? Anyway, for the problem you described here: Maybe it's possible to just use 'ofport_request' configuration option for interfaces? It could be implemented as picking a unique ofport before adding a new port and set 'ofport_request' along with port creation. Another way is to fix current ofport for interfaces once OVS chose it, i.e. port add -> check ofport -> set 'ofport_request' with that value. In any case after reboot OVS will choose exactly same ofport as it was before because it is stored in database. What do you think? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
> > I've been taking a look at this patch for the last few minutes. It > introduces a lot > of mechanism for the use case. Did you consider any simpler mechanisms to > achieve the same effect? What prevented them from working? > I agree Ben. This change does bring some complexity to the code. A simpler solution would be to leave the ofport number in ovsdb record of the interface (instead of setting it to -1). But issue was when the port delete happens we do not see this record to be able to free the ofport number. Is there a way to handle this? Warm Regards, Vishal Ajmera ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
On Thu, Jun 13, 2019 at 08:28:02AM +, Vishal Deep Ajmera wrote: > > > > > This patch fixes both the above issue by retaining the ofport number till > > ofport > > entry exists in the OVSDB. > > > > Warm Regards, > > Vishal Ajmera > > Hi Ben, > > Did you get a chance to review this patch ? I've been taking a look at this patch for the last few minutes. It introduces a lot of mechanism for the use case. Did you consider any simpler mechanisms to achieve the same effect? What prevented them from working? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
> > This patch fixes both the above issue by retaining the ofport number till > ofport > entry exists in the OVSDB. > > Warm Regards, > Vishal Ajmera Hi Ben, Did you get a chance to review this patch ? Warm Regards, Vishal Ajmera ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
> > > > This patch reserves the ofport port number for the duration the ofport > > entry persists in the ovsdb. Once the entry is deleted from ovsdb the > > ofport number is recirculated on LRU basis. > > > > Signed-off-by: Vishal Deep Ajmera > > This is an interesting patch and I like the idea. The implementation does add > some conceptual complexity. > > Can you say some more about the scenario that this covers? It isn't one that > I've > heard come up very much in the past. Does it arise in real situations that > you've > encountered? Hi Ben, Thanks for looking into this patch. We have 2 scenarios in our deployment where we encountered issue. Scenario 1: (SDN Controller - ODL, OpenStack - Pike) Assume we have 4 ports in ovs on "netdev" bridge br-int: tap0 (1) tap1 (2) vhu1 (3) vhu2 (4) () denotes the ofport number allocated to the port by OVS. If the host compute is rebooted, the tap devices tap0 and tap1 are not recreated (or atleast not immediately). When OVS restarts the corresponding tap interfaces are in error state: "could not open network device tap0 (No such device)". Unfortunately due to this error, OVS sets the ofport field to -1 in OVSDB which means these ofport numbers (1) and (2) are now available to be allocated to any new port. At this time, SDN controller will need to remove all the flows pertaining to tap ports as ofport numbers are no longer same. A peculiar behavior we observed with OpenStack Pike was that when compute was restarted, it also triggered deletes and adds of all VHU ports (and not as a single transaction). This also resulted in change of existing vhu's ofport number. After compute reboot: tap0 (-1) -> "could not open network device tap0 (No such device)" tap1 (-1) -> "could not open network device tap1 (No such device)" vhu1 (3) vhu2 (4) Then a delete/add of vhu port changes the ofport allocation as follows: tap0 (-1) tap1 (-1) vhu1 (1) vhu2 (2) Things got messed up here as SDN controller on one hand trying to remove flows for tap0 /tap1 and on the other also modifying flows for vhu1 and vhu2. Unfortunately both tap0 and vhu1 has ofport (1) and tap1 and vhu2 has ofport (2). Until the time flows are properly cleaned by SDN controller we end up in stale flows of tap ports being used for vhu ports. If OVS avoids giving same ofport number (1) and (2) to existing VHU ports or any new port, this situation could be avoided. Scenario 2: (SDN Controller - ODL, OpenStack - Pike) We have another scenario in our deployment, where in we always start OVS with DPDK enabled. But if DPDK initialization fails due to any error, we fall back to non-dpdk mode and start OVS. This is to make sure we still manage to have connectivity with the computes. But this causes all VHU's and DPDK ports to fail and frees up their ofport numbers. Once we correct the underlying DPDK issue and start OVS again with dpdk mode, SDN controller is now required to reconstruct (and not just replay) all the flows which it had earlier as the ofport numbers are not guaranteed to be same again. This patch fixes both the above issue by retaining the ofport number till ofport entry exists in the OVSDB. Warm Regards, Vishal Ajmera ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
On Tue, Jun 04, 2019 at 11:47:57AM +, Vishal Deep Ajmera wrote: > Current OVS implementation frees the ofport number allocated for > a given ofport if the underlying interface could not get initialized > due to any error. However it does not delete the entry of ofport from > ovsdb. This means any new ofport can get this ofport number anytime > in future. When the underlying interface issue is resolved for the > ofport it gets a new ofport number from the pool. This has an > undesired effect where any SDN controller will have to modify all the > flows pertaining this ofport due to change in ofport number and > reprogram the flows in the switch. > > This patch reserves the ofport port number for the duration the ofport > entry persists in the ovsdb. Once the entry is deleted from ovsdb the > ofport number is recirculated on LRU basis. > > Signed-off-by: Vishal Deep Ajmera This is an interesting patch and I like the idea. The implementation does add some conceptual complexity. Can you say some more about the scenario that this covers? It isn't one that I've heard come up very much in the past. Does it arise in real situations that you've encountered? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ofproto: Preserve ofport number for failed ofport across restarts
Current OVS implementation frees the ofport number allocated for
a given ofport if the underlying interface could not get initialized
due to any error. However it does not delete the entry of ofport from
ovsdb. This means any new ofport can get this ofport number anytime
in future. When the underlying interface issue is resolved for the
ofport it gets a new ofport number from the pool. This has an
undesired effect where any SDN controller will have to modify all the
flows pertaining this ofport due to change in ofport number and
reprogram the flows in the switch.
This patch reserves the ofport port number for the duration the ofport
entry persists in the ovsdb. Once the entry is deleted from ovsdb the
ofport number is recirculated on LRU basis.
Signed-off-by: Vishal Deep Ajmera
---
ofproto/ofproto.c | 17 +--
ofproto/ofproto.h | 2 +
vswitchd/bridge.c | 145 +++---
3 files changed, 132 insertions(+), 32 deletions(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 1d6fc00..df77859 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -190,8 +190,6 @@ static void reinit_ports(struct ofproto *);
static long long int ofport_get_usage(const struct ofproto *,
ofp_port_t ofp_port);
-static void ofport_set_usage(struct ofproto *, ofp_port_t ofp_port,
- long long int last_used);
static void ofport_remove_usage(struct ofproto *, ofp_port_t ofp_port);
/* Ofport usage.
@@ -2264,10 +2262,23 @@ static ofp_port_t
alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
{
uint16_t port_idx;
+struct ofport *port;
port_idx = simap_get(>ofp_requests, netdev_name);
port_idx = port_idx ? port_idx : UINT16_MAX;
+/* Check if port_idx is not allocated to any other interface.
+ * If available, then reserve for given netdev and skip
+ * allocation algorithm. */
+if (port_idx != UINT16_MAX) {
+port = ofproto_get_port(ofproto, u16_to_ofp(port_idx));
+if ((!port) || (port->netdev != NULL &&
+!strcmp(netdev_name, netdev_get_name(port->netdev {
+ofport_set_usage(ofproto, u16_to_ofp(port_idx), LLONG_MAX);
+return u16_to_ofp(port_idx);
+}
+}
+
if (port_idx >= ofproto->max_ports
|| ofport_get_usage(ofproto, u16_to_ofp(port_idx)) == LLONG_MAX) {
uint16_t lru_ofport = 0, end_port_no = ofproto->alloc_port_no;
@@ -2577,7 +2588,7 @@ ofport_get_usage(const struct ofproto *ofproto,
ofp_port_t ofp_port)
return 0;
}
-static void
+void
ofport_set_usage(struct ofproto *ofproto, ofp_port_t ofp_port,
long long int last_used)
{
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 6e4afff..c236e0b 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -313,6 +313,8 @@ const char *ofproto_port_open_type(const struct ofproto *,
const char *port_type);
int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t *ofp_portp);
int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port);
+void ofport_set_usage(struct ofproto *, ofp_port_t ofp_port,
+ long long int last_used);
void ofproto_port_set_config(struct ofproto *, ofp_port_t ofp_port,
const struct smap *cfg);
int ofproto_port_get_stats(const struct ofport *, struct netdev_stats *stats);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 0702cc6..4852ef0 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -137,6 +137,8 @@ struct bridge {
/* Used during reconfiguration. */
struct shash wanted_ports;
+struct hmap failed_ports; /* List of ports for which interface
+ * add failed. */
/* Synthetic local port if necessary. */
struct ovsrec_port synth_local_port;
@@ -273,10 +275,17 @@ static bool port_is_bond_fake_iface(const struct port *);
static unixctl_cb_func qos_unixctl_show_types;
static unixctl_cb_func qos_unixctl_show;
-static struct port *port_create(struct bridge *, const struct ovsrec_port *);
-static void port_del_ifaces(struct port *);
+static struct port *port_create(struct bridge *, const struct ovsrec_port *,
+bool stale);
+static void port_del_ifaces(struct port *, bool stale);
static void port_destroy(struct port *);
-static struct port *port_lookup(const struct bridge *, const char *name);
+static struct port *port_lookup(const struct bridge *, const char *name,
+bool stale);
+
+static void failed_port_destroy(struct port *);
+static void failed_iface_insert(struct port *port,
+const struct ovsrec_interface *iface_cfg);
+
static void port_configure(struct port *);
static struct lacp_settings *port_configure_lacp(struct port *,
struct
