| Message ID | 20240202-dei-member-v1-2-ebdff93fc682@ovn.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | vswitch.xml: Use member wording for bonds. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 2/2/24 16:38, Simon Horman wrote: > Since the patch-set that included [1] there has been a policy of using > the term member for bonds, LACP, and bundle contexts. This is > consistent with the more recently adopted policy of using the inclusive > naming word list v1 [2, 3]. > > This patch addresses the name of the bond_active_member ovsdb column. > This is used internally to allow the active member of a bond across OVS > restart. And the effect of this patch is that when restarting OVS, if > the instance before restart did not have this patch and the instance > after does, or vice-versa, then active bond member selection will not be > sticky across that restart. Otherwise, the existing behaviour is > maintained. Hi, Simon. Unfortunately it is not possible to remove existing columns from the database. The new schema is not compatible and the existing clients will fail to connect to the updated database. You may see that if you only update the schema, but not re-compile ovs-vswitchd: 2024-02-02T16:43:00.228Z|00027|ovsdb_cs|INFO|unix:sandbox/db.sock: received unexpected error response in DATA_MONITOR_REQUESTED state: {"error":{"details":"bond_active_slave is not a valid column name", "error":"syntax error", "syntax":"[\"bond_active_slave\", \"bond_downdelay\",\"bond_fake_iface\", \"bond_mode\",\"bond_updelay\",\"cvlans\",\"fake_bridge\", \"interfaces\",\"lacp\",\"mac\",\"name\",\"other_config\", \"protected\",\"qos\",\"rstp_statistics\",\"rstp_status\", \"statistics\",\"status\",\"tag\",\"trunks\",\"vlan_mode\"]" },"id":52,"result":null} The column have to stay, similarly to other public interfaces. What we can probably do is to add a new column and use/store values from/into both, while removing the old one from the documentation. Best regards, Ilya Maximets.
On Fri, Feb 02, 2024 at 05:48:03PM +0100, Ilya Maximets wrote: > On 2/2/24 16:38, Simon Horman wrote: > > Since the patch-set that included [1] there has been a policy of using > > the term member for bonds, LACP, and bundle contexts. This is > > consistent with the more recently adopted policy of using the inclusive > > naming word list v1 [2, 3]. > > > > This patch addresses the name of the bond_active_member ovsdb column. > > This is used internally to allow the active member of a bond across OVS > > restart. And the effect of this patch is that when restarting OVS, if > > the instance before restart did not have this patch and the instance > > after does, or vice-versa, then active bond member selection will not be > > sticky across that restart. Otherwise, the existing behaviour is > > maintained. > > Hi, Simon. Unfortunately it is not possible to remove existing columns > from the database. The new schema is not compatible and the existing > clients will fail to connect to the updated database. You may see that > if you only update the schema, but not re-compile ovs-vswitchd: > > 2024-02-02T16:43:00.228Z|00027|ovsdb_cs|INFO|unix:sandbox/db.sock: > received unexpected error response in DATA_MONITOR_REQUESTED state: > {"error":{"details":"bond_active_slave is not a valid column name", > "error":"syntax error", > "syntax":"[\"bond_active_slave\", \"bond_downdelay\",\"bond_fake_iface\", > \"bond_mode\",\"bond_updelay\",\"cvlans\",\"fake_bridge\", > \"interfaces\",\"lacp\",\"mac\",\"name\",\"other_config\", > \"protected\",\"qos\",\"rstp_statistics\",\"rstp_status\", > \"statistics\",\"status\",\"tag\",\"trunks\",\"vlan_mode\"]" > },"id":52,"result":null} > > > The column have to stay, similarly to other public interfaces. > > What we can probably do is to add a new column and use/store values > from/into both, while removing the old one from the documentation. Thanks, and sorry for not noticing the problem this introduces. I'll work on the approach that you have suggested.
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 95a65fcdcd5e..01ec5e183091 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -463,7 +463,7 @@ bridge_init(const char *remote) ovsdb_idl_omit_alert(idl, &ovsrec_port_col_rstp_status); ovsdb_idl_omit_alert(idl, &ovsrec_port_col_rstp_statistics); ovsdb_idl_omit_alert(idl, &ovsrec_port_col_statistics); - ovsdb_idl_omit_alert(idl, &ovsrec_port_col_bond_active_slave); + ovsdb_idl_omit_alert(idl, &ovsrec_port_col_bond_active_member); ovsdb_idl_omit(idl, &ovsrec_port_col_external_ids); ovsdb_idl_omit_alert(idl, &ovsrec_port_col_trunks); ovsdb_idl_omit_alert(idl, &ovsrec_port_col_vlan_mode); @@ -3003,7 +3003,7 @@ port_refresh_bond_status(struct port *port, bool force_update) ds_init(&mac_s); ds_put_format(&mac_s, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); - ovsrec_port_set_bond_active_slave(port->cfg, ds_cstr(&mac_s)); + ovsrec_port_set_bond_active_member(port->cfg, ds_cstr(&mac_s)); ds_destroy(&mac_s); } } @@ -4640,7 +4640,7 @@ port_configure_bond(struct port *port, struct bond_settings *s) netdev_set_miimon_interval(iface->netdev, miimon_interval); } - mac_s = port->cfg->bond_active_slave; + mac_s = port->cfg->bond_active_member; if (!mac_s || !ovs_scan(mac_s, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(s->active_member_mac))) { /* OVSDB did not store the last active interface */ diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema index e2d5e2e85e60..bfa9f39afb49 100644 --- a/vswitchd/vswitch.ovsschema +++ b/vswitchd/vswitch.ovsschema @@ -1,6 +1,6 @@ {"name": "Open_vSwitch", - "version": "8.5.0", - "cksum": "4040946650 27557", + "version": "9.0.0", + "cksum": "3156488466 27558", "tables": { "Open_vSwitch": { "columns": { @@ -194,7 +194,7 @@ "type": "integer"}, "bond_downdelay": { "type": "integer"}, - "bond_active_slave": { + "bond_active_member": { "type": {"key": {"type": "string"}, "min": 0, "max": 1}}, "bond_fake_iface": { diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 8a1b607d71b9..5bf5a4107b5b 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2557,7 +2557,7 @@ </column> </group> - <column name="bond_active_slave"> + <column name="bond_active_member"> For a bonded port, record the MAC address of the current active member. </column>
Since the patch-set that included [1] there has been a policy of using the term member for bonds, LACP, and bundle contexts. This is consistent with the more recently adopted policy of using the inclusive naming word list v1 [2, 3]. This patch addresses the name of the bond_active_member ovsdb column. This is used internally to allow the active member of a bond across OVS restart. And the effect of this patch is that when restarting OVS, if the instance before restart did not have this patch and the instance after does, or vice-versa, then active bond member selection will not be sticky across that restart. Otherwise, the existing behaviour is maintained. [1] 91fc374a9c5a ("Eliminate use of term "slave" in bond, LACP, and bundle contexts.") [2] df5e5cf4318a ("Documentation: Add section on inclusive language.") [3] https://inclusivenaming.org/word-lists/ [4] 3e5aeeb581fa ("bridge: Keep bond active slave selection across OVS restart") Signed-off-by: Simon Horman <horms@ovn.org> --- vswitchd/bridge.c | 6 +++--- vswitchd/vswitch.ovsschema | 6 +++--- vswitchd/vswitch.xml | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)