| Message ID | 20240412015727.4152034-5-ihrachys@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Correct tunnel ids exhaustion scenario. | 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 |
On 4/11/24 21:57, Ihar Hrachyshka wrote: > This allows callers to avoid cleanup of the record in case the function > fails. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > northd/northd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 4cea669cf..6a8ace52f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4322,6 +4322,9 @@ ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn, > } > /* Assign new tunnel ids where needed. */ > if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { > + if (!sb) { > + sbrec_port_binding_delete(op->sb); > + } Given how the code is structured, I think this could be simplified a bit. The code above this point is: if (sb) { /* We don't care about this case */ } else { op->sb = sbrec_port_binding_insert(ovnsb_txn); sbrec_port_binding_set_logical_port(op->sb, op->key); } Could the else block be restructured as follows? if (sb) { /* We don't care about this case */ } else { if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { return false; } op->sb = sbrec_port_binding_insert(ovnsb_txn); sbrec_port_binding_set_logical_port(op->sb, op->key); } This way, we wait to insert the SB port binding until after we successfully allocate the tunnel key. We don't insert it and then immediately delete it this way. > return false; > } > ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name, > @@ -4345,9 +4348,6 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports, > if (!ls_port_init(op, ovnsb_txn, od, sb, > sbrec_mirror_table, sbrec_chassis_table, > sbrec_chassis_by_name, sbrec_chassis_by_hostname)) { > - if (op->sb) { > - sbrec_port_binding_delete(op->sb); > - } > ovn_port_destroy(ls_ports, op); > return NULL; > } > @@ -4551,8 +4551,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > ni->sbrec_chassis_table, > ni->sbrec_chassis_by_name, > ni->sbrec_chassis_by_hostname)) { > - if (op->sb) { > - sbrec_port_binding_delete(op->sb); > + if (sb) { > + sbrec_port_binding_delete(sb); > } > ovn_port_destroy(&nd->ls_ports, op); > goto fail;
On Tue, Apr 23, 2024 at 4:59 PM Mark Michelson <mmichels@redhat.com> wrote: > On 4/11/24 21:57, Ihar Hrachyshka wrote: > > This allows callers to avoid cleanup of the record in case the function > > fails. > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > --- > > northd/northd.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 4cea669cf..6a8ace52f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -4322,6 +4322,9 @@ ls_port_init(struct ovn_port *op, struct > ovsdb_idl_txn *ovnsb_txn, > > } > > /* Assign new tunnel ids where needed. */ > > if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { > > + if (!sb) { > > + sbrec_port_binding_delete(op->sb); > > + } > > Given how the code is structured, I think this could be simplified a > bit. The code above this point is: > > if (sb) { > /* We don't care about this case */ > } else { > op->sb = sbrec_port_binding_insert(ovnsb_txn); > sbrec_port_binding_set_logical_port(op->sb, op->key); > } > > Could the else block be restructured as follows? > > if (sb) { > /* We don't care about this case */ > } else { > if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { > return false; > } > op->sb = sbrec_port_binding_insert(ovnsb_txn); > sbrec_port_binding_set_logical_port(op->sb, op->key); > } > > This way, we wait to insert the SB port binding until after we > successfully allocate the tunnel key. We don't insert it and then > immediately delete it this way. > We would need to call ovn_port_allocate_key() for `if (sb)` branch too then (it's needed in both cases.) But I think what we can do to simplify this code a bit is reversing the order as follows: ``` if (sb) { ... } if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { return false; } if (!sb) { op->sb = sbrec_port_binding_insert(ovnsb_txn); sbrec_port_binding_set_logical_port(op->sb, op->key); } ... ``` I will test it and post an update. Thanks for considering the series, Mark! > > > return false; > > } > > ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name, > > @@ -4345,9 +4348,6 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, > struct hmap *ls_ports, > > if (!ls_port_init(op, ovnsb_txn, od, sb, > > sbrec_mirror_table, sbrec_chassis_table, > > sbrec_chassis_by_name, > sbrec_chassis_by_hostname)) { > > - if (op->sb) { > > - sbrec_port_binding_delete(op->sb); > > - } > > ovn_port_destroy(ls_ports, op); > > return NULL; > > } > > @@ -4551,8 +4551,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > ni->sbrec_chassis_table, > > ni->sbrec_chassis_by_name, > > ni->sbrec_chassis_by_hostname)) { > > - if (op->sb) { > > - sbrec_port_binding_delete(op->sb); > > + if (sb) { > > + sbrec_port_binding_delete(sb); > > } > > ovn_port_destroy(&nd->ls_ports, op); > > goto fail; > >
diff --git a/northd/northd.c b/northd/northd.c index 4cea669cf..6a8ace52f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4322,6 +4322,9 @@ ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn, } /* Assign new tunnel ids where needed. */ if (!ovn_port_allocate_key(sbrec_chassis_table, op)) { + if (!sb) { + sbrec_port_binding_delete(op->sb); + } return false; } ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name, @@ -4345,9 +4348,6 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports, if (!ls_port_init(op, ovnsb_txn, od, sb, sbrec_mirror_table, sbrec_chassis_table, sbrec_chassis_by_name, sbrec_chassis_by_hostname)) { - if (op->sb) { - sbrec_port_binding_delete(op->sb); - } ovn_port_destroy(ls_ports, op); return NULL; } @@ -4551,8 +4551,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, ni->sbrec_chassis_table, ni->sbrec_chassis_by_name, ni->sbrec_chassis_by_hostname)) { - if (op->sb) { - sbrec_port_binding_delete(op->sb); + if (sb) { + sbrec_port_binding_delete(sb); } ovn_port_destroy(&nd->ls_ports, op); goto fail;
This allows callers to avoid cleanup of the record in case the function fails. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- northd/northd.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)