CP-23093: Implement toggle of IGMP snooping on xcp-networkd #107
Conversation
|
@robhoes -- Would you please review this PR? We also need to add a field |
fillzero
force-pushed
the
CP-23093
branch
2 times, most recently
from
415cbd0
to
62d5947
Compare
|
This PR depends on xapi-project/xcp-idl#164 |
networkd/network_server.ml
Outdated
| update_config bridge_name c; | ||
| exec (fun () -> | ||
| create () dbg ?vlan ?mac:bridge_mac ~other_config ~name:bridge_name (); | ||
| create () dbg ?vlan ?mac:bridge_mac ?igmp_snooping:igmp_snooping ~other_config ~name:bridge_name (); |
There was a problem hiding this comment.
use ~igmp_snooping better than ?igmp_snooping:igmp_snooping
networkd/network_server.ml
Outdated
| @@ -949,6 +949,12 @@ module Bridge = struct | |||
| update_config name {(get_config name) with persistent_b = value} | |||
| ) () | |||
|
|
|||
| let set_ovs_igmp_snooping _ dbg ~name ~value = | |||
There was a problem hiding this comment.
We don't need this function, because we can just call create even if the bridge already exists. All functions in the interface, including make_config and create are idempotent, which means that you can safely call them multiple times. The job of Bridge.make_config is to update the current configuration of the bridge based on the new configuration that is given, and only do something when (part of) the configuration has changed.
There was a problem hiding this comment.
@robhoes Thanks for your comments, updated PR accordingly
networkd/network_server.ml
Outdated
| Debug.with_thread_associated dbg (fun () -> | ||
| debug "Creating bridge %s%s" name (match vlan with | ||
| | None -> "" | ||
| | Some (parent, vlan) -> Printf.sprintf " (VLAN %d on bridge %s)" vlan parent | ||
| ); | ||
| Stdext.Opt.iter (destroy_existing_vlan_bridge name) vlan; | ||
| update_config name {(get_config name) with vlan; bridge_mac=mac; other_config}; | ||
| update_config name {(get_config name) with vlan; bridge_mac=mac; igmp_snooping=igmp_snooping; other_config}; |
There was a problem hiding this comment.
This can be just igmp_snooping (because the parameter has the same name).
lib/network_utils.ml
Outdated
| | Some igmp_snooping -> | ||
| if vlan = None then | ||
| ["--"; "set"; "bridge"; name; "mcast_snooping_enable=" ^ (string_of_bool igmp_snooping)] | ||
| else | ||
| [] |
There was a problem hiding this comment.
In fact, we can easily combine the match and if:
match igmp_snooping, vlan with
| Some x, None -> ["--"; "set"; "bridge"; name; "mcast_snooping_enable=" ^ (string_of_bool x)]
| _ -> []
There was a problem hiding this comment.
Thanks. Updated accordingly.
Signed-off-by: Liang Dai <liang.dai1@citrix.com>
|
This is fine now, but we can't merge it without the matching change in xcp-idl, and we can't merge that without a change to xen-api, for which there is no PR yet. |
|
Thanks @robhoes . Regarding the xen-api change, @YarsinCitrix will send PR later. Also copy @huizh to be aware. |
|
I'll close this for now. When the xen-api patch is ready, I'll reopen and merge this PR. |
|
Hi @robhoes, thanks for the review and the suggestion. Very helpful. Will follow up your suggestion to raise a XenAPI PR so that to get this and xapi-project/xcp-idl#164 merged. |
Signed-off-by: Liang Dai liang.dai1@citrix.com