| Message ID | 20230607172834.3851733-2-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/2] qos: fix potential double deletion of ovs idl row | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
> If iface does not exist when qos I+P is run the first time > a qos queue is still created and qos I+P will never apply qos to OVS > qos might still be applied later, if/when (for any reason) runtime_date must be recomputed > > Fixes: 7d1d111ff213 ("controller: configure qos through ovs qos table and do not run tc directly") > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/binding.c | 28 +++++++++++++++------------- > tests/ovn.at | 7 +++++++ > 2 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index f2896a9c9..8a0a24b95 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -362,7 +362,7 @@ configure_qos(const struct sbrec_port_binding *pb, > struct qos_queue *q = find_qos_queue(b_ctx_out->qos_map, hash, > pb->logical_port); > if (!q || q->min_rate != min_rate || q->max_rate != max_rate || > - q->burst != burst) { > + q->burst != burst || strcmp(network, q->network)) { I think network can be NULL here. > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, > &bridge_mappings); > @@ -378,22 +378,24 @@ configure_qos(const struct sbrec_port_binding *pb, > add_ovs_qos_table_entry(b_ctx_in->ovs_idl_txn, port, min_rate, > max_rate, burst, queue_id, > pb->logical_port); > + if (!q) { > + q = xzalloc(sizeof *q); > + hmap_insert(b_ctx_out->qos_map, &q->node, hash); > + q->port = xstrdup(pb->logical_port); > + q->queue_id = queue_id; > + } > + free(q->network); > + q->network = network ? xstrdup(network) : NULL; > + q->min_rate = min_rate; > + q->max_rate = max_rate; > + q->burst = burst; > + } else { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_DBG_RL(&rl, "no iface => no entries added on qos_table"); > } > shash_destroy(&bridge_mappings); what about just returning if iface is NULL here? Regards, Lorenzo > } > > - if (!q) { > - q = xzalloc(sizeof *q); > - hmap_insert(b_ctx_out->qos_map, &q->node, hash); > - q->port = xstrdup(pb->logical_port); > - q->queue_id = queue_id; > - } > - > - free(q->network); > - q->network = network ? xstrdup(network) : NULL; > - q->min_rate = min_rate; > - q->max_rate = max_rate; > - q->burst = burst; > } > > static const struct ovsrec_queue * > diff --git a/tests/ovn.at b/tests/ovn.at > index 05bf7fa5b..70ce91d26 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -36089,6 +36089,7 @@ check ovn-nbctl ls-add ls1 > check ovn-nbctl lsp-add ls1 public1 > check ovn-nbctl lsp-set-addresses public1 unknown > check ovn-nbctl lsp-set-type public1 localnet > +check ovn-nbctl lsp-set-options public1 network_name=phys > > check ovn-nbctl lsp-add ls1 lsp5 > check ovn-nbctl lsp-set-addresses lsp5 f0:00:00:00:00:05 > @@ -36098,6 +36099,12 @@ ovs-vsctl add-port br-int vif5 -- \ > ofport-request=5 > OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp5` = xup]) > > +# Delete and add back public1 > +check ovn-nbctl --wait=hv lsp-del public1 > +check ovn-nbctl lsp-add ls1 public1 > +check ovn-nbctl lsp-set-addresses public1 unknown > +check ovn-nbctl lsp-set-type public1 localnet > + > check ovn-nbctl set Logical_Switch_Port public1 options:qos_min_rate=6000000000 > check ovn-nbctl set Logical_Switch_Port public1 options:qos_max_rate=7000000000 > check ovn-nbctl set Logical_Switch_Port public1 options:qos_burst=8000000000 > -- > 2.31.1 >
On Wed, Jun 7, 2023 at 8:16 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > If iface does not exist when qos I+P is run the first time > > a qos queue is still created and qos I+P will never apply qos to OVS > > qos might still be applied later, if/when (for any reason) runtime_date > must be recomputed > > > > Fixes: 7d1d111ff213 ("controller: configure qos through ovs qos table > and do not run tc directly") > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > controller/binding.c | 28 +++++++++++++++------------- > > tests/ovn.at | 7 +++++++ > > 2 files changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index f2896a9c9..8a0a24b95 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -362,7 +362,7 @@ configure_qos(const struct sbrec_port_binding *pb, > > struct qos_queue *q = find_qos_queue(b_ctx_out->qos_map, hash, > > pb->logical_port); > > if (!q || q->min_rate != min_rate || q->max_rate != max_rate || > > - q->burst != burst) { > > + q->burst != burst || strcmp(network, q->network)) { > > I think network can be NULL here. > You are right. Sending a v2. > > > struct shash bridge_mappings = > SHASH_INITIALIZER(&bridge_mappings); > > add_ovs_bridge_mappings(b_ctx_in->ovs_table, > b_ctx_in->bridge_table, > > &bridge_mappings); > > @@ -378,22 +378,24 @@ configure_qos(const struct sbrec_port_binding *pb, > > add_ovs_qos_table_entry(b_ctx_in->ovs_idl_txn, port, > min_rate, > > max_rate, burst, queue_id, > > pb->logical_port); > > + if (!q) { > > + q = xzalloc(sizeof *q); > > + hmap_insert(b_ctx_out->qos_map, &q->node, hash); > > + q->port = xstrdup(pb->logical_port); > > + q->queue_id = queue_id; > > + } > > + free(q->network); > > + q->network = network ? xstrdup(network) : NULL; > > + q->min_rate = min_rate; > > + q->max_rate = max_rate; > > + q->burst = burst; > > + } else { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 1); > > + VLOG_DBG_RL(&rl, "no iface => no entries added on > qos_table"); > > } > > shash_destroy(&bridge_mappings); > > what about just returning if iface is NULL here? > Do you mean "do not add the extra debug logging" ? If this is what you mean, I agree, it is not very useful, so will delete in v2. > > Regards, > Lorenzo > > Thanks! Xavier > > } > > > > - if (!q) { > > - q = xzalloc(sizeof *q); > > - hmap_insert(b_ctx_out->qos_map, &q->node, hash); > > - q->port = xstrdup(pb->logical_port); > > - q->queue_id = queue_id; > > - } > > - > > - free(q->network); > > - q->network = network ? xstrdup(network) : NULL; > > - q->min_rate = min_rate; > > - q->max_rate = max_rate; > > - q->burst = burst; > > } > > > > static const struct ovsrec_queue * > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 05bf7fa5b..70ce91d26 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -36089,6 +36089,7 @@ check ovn-nbctl ls-add ls1 > > check ovn-nbctl lsp-add ls1 public1 > > check ovn-nbctl lsp-set-addresses public1 unknown > > check ovn-nbctl lsp-set-type public1 localnet > > +check ovn-nbctl lsp-set-options public1 network_name=phys > > > > check ovn-nbctl lsp-add ls1 lsp5 > > check ovn-nbctl lsp-set-addresses lsp5 f0:00:00:00:00:05 > > @@ -36098,6 +36099,12 @@ ovs-vsctl add-port br-int vif5 -- \ > > ofport-request=5 > > OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp5` = xup]) > > > > +# Delete and add back public1 > > +check ovn-nbctl --wait=hv lsp-del public1 > > +check ovn-nbctl lsp-add ls1 public1 > > +check ovn-nbctl lsp-set-addresses public1 unknown > > +check ovn-nbctl lsp-set-type public1 localnet > > + > > check ovn-nbctl set Logical_Switch_Port public1 > options:qos_min_rate=6000000000 > > check ovn-nbctl set Logical_Switch_Port public1 > options:qos_max_rate=7000000000 > > check ovn-nbctl set Logical_Switch_Port public1 > options:qos_burst=8000000000 > > -- > > 2.31.1 > > >
diff --git a/controller/binding.c b/controller/binding.c index f2896a9c9..8a0a24b95 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -362,7 +362,7 @@ configure_qos(const struct sbrec_port_binding *pb, struct qos_queue *q = find_qos_queue(b_ctx_out->qos_map, hash, pb->logical_port); if (!q || q->min_rate != min_rate || q->max_rate != max_rate || - q->burst != burst) { + q->burst != burst || strcmp(network, q->network)) { struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, &bridge_mappings); @@ -378,22 +378,24 @@ configure_qos(const struct sbrec_port_binding *pb, add_ovs_qos_table_entry(b_ctx_in->ovs_idl_txn, port, min_rate, max_rate, burst, queue_id, pb->logical_port); + if (!q) { + q = xzalloc(sizeof *q); + hmap_insert(b_ctx_out->qos_map, &q->node, hash); + q->port = xstrdup(pb->logical_port); + q->queue_id = queue_id; + } + free(q->network); + q->network = network ? xstrdup(network) : NULL; + q->min_rate = min_rate; + q->max_rate = max_rate; + q->burst = burst; + } else { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_DBG_RL(&rl, "no iface => no entries added on qos_table"); } shash_destroy(&bridge_mappings); } - if (!q) { - q = xzalloc(sizeof *q); - hmap_insert(b_ctx_out->qos_map, &q->node, hash); - q->port = xstrdup(pb->logical_port); - q->queue_id = queue_id; - } - - free(q->network); - q->network = network ? xstrdup(network) : NULL; - q->min_rate = min_rate; - q->max_rate = max_rate; - q->burst = burst; } static const struct ovsrec_queue * diff --git a/tests/ovn.at b/tests/ovn.at index 05bf7fa5b..70ce91d26 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36089,6 +36089,7 @@ check ovn-nbctl ls-add ls1 check ovn-nbctl lsp-add ls1 public1 check ovn-nbctl lsp-set-addresses public1 unknown check ovn-nbctl lsp-set-type public1 localnet +check ovn-nbctl lsp-set-options public1 network_name=phys check ovn-nbctl lsp-add ls1 lsp5 check ovn-nbctl lsp-set-addresses lsp5 f0:00:00:00:00:05 @@ -36098,6 +36099,12 @@ ovs-vsctl add-port br-int vif5 -- \ ofport-request=5 OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp5` = xup]) +# Delete and add back public1 +check ovn-nbctl --wait=hv lsp-del public1 +check ovn-nbctl lsp-add ls1 public1 +check ovn-nbctl lsp-set-addresses public1 unknown +check ovn-nbctl lsp-set-type public1 localnet + check ovn-nbctl set Logical_Switch_Port public1 options:qos_min_rate=6000000000 check ovn-nbctl set Logical_Switch_Port public1 options:qos_max_rate=7000000000 check ovn-nbctl set Logical_Switch_Port public1 options:qos_burst=8000000000
If iface does not exist when qos I+P is run the first time a qos queue is still created and qos I+P will never apply qos to OVS qos might still be applied later, if/when (for any reason) runtime_date must be recomputed Fixes: 7d1d111ff213 ("controller: configure qos through ovs qos table and do not run tc directly") Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/binding.c | 28 +++++++++++++++------------- tests/ovn.at | 7 +++++++ 2 files changed, 22 insertions(+), 13 deletions(-)