Network: Add OVS bridge information #15044
Conversation
ghadi-rahme
force-pushed
the
adding-ovs-bridge-info
branch
2 times, most recently
from
bf95f29 to
ce0e829
Compare
markylaing
changed the title
Size increase justificationFunctions were added to the |
IIRC we dont want lxd-agent needing to depend on ovs package |
There was a problem hiding this comment.
Overall I'm happy to add these functions to the openvswitch package but the parsing of ovs-appctl stp/show output could use some work. Ideally a format can be specified or ovs-vsctl can be used instead. Additionally, I think the best long-term solution for having lxd/resources as a dependency will be to refactor the package so that it does not import state.State.
lxd/network/openvswitch/ovs.go
Outdated
| // Default STP priority is 32768 (0x8000). This value will be used if the | ||
| // priority cannot be retrieved or if STP is disabled | ||
| func GetStpPriority(bridgeName string) (string, error) { | ||
| output, err := shared.RunCommand("ovs-appctl", "stp/show", bridgeName) |
There was a problem hiding this comment.
I think RunCommand is deprecated. Instead you can use RunCommandContext and pass a context.Context into this method.
lxd/network/openvswitch/ovs.go
Outdated
| // priority cannot be retrieved or if STP is disabled | ||
| func GetStpPriority(bridgeName string) (string, error) { | ||
| output, err := shared.RunCommand("ovs-appctl", "stp/show", bridgeName) | ||
| if err != nil { |
There was a problem hiding this comment.
Please can you use errors.Is and/or errors.As for these kind of checks.
lxd/network/openvswitch/ovs.go
Outdated
| return "8000", nil | ||
| } | ||
| } | ||
| return "8000", err |
There was a problem hiding this comment.
You will always need a newline after a closing brace and before any subsequent statements.
lxd/network/openvswitch/ovs.go
Outdated
| return "8000", err | ||
| } | ||
| hexStp := fmt.Sprintf("%4X", decimalStp) | ||
| return hexStp, nil |
There was a problem hiding this comment.
Why do we return the STP as a hex string?
There was a problem hiding this comment.
The reason is because the IEEE802.1D standard represents the STP priority in binary terms. Linux (and many switches) already uses the hexadecimal format to represent them but OVS defaults to showing the value in base 10. Also to represent a bridge ID which is a combination of the STP priority and the MAC address [1], the STP will need to be in hex format just like the MAC address is. In fact IEEE802.1D is the same standard that defines the MAC address as also being represented in binary format (hexadecimal is used on MAC addresses to simplify the representation, the same is done for STP priority).
[1] https://www.kernel.org/doc/Documentation/networking/bridge.rst
Bridge ID: The Bridge ID is composed of two components: Bridge Priority
and the MAC address of the bridge. It uniquely identifies each bridge
in the network. The Bridge ID is used to compare the priorities of
different bridges.
There was a problem hiding this comment.
Thanks, can you add this reasoning to a comment?
lxd/network/openvswitch/ovs.go
Outdated
| var stpPriority string | ||
| var bridgeSection bool | ||
| lines := strings.Split(output, "\n") | ||
|
|
||
| for _, line := range lines { | ||
| line = strings.TrimSpace(line) | ||
| if line == "Bridge ID:" { | ||
| bridgeSection = true | ||
| } | ||
| if bridgeSection { | ||
| if strings.HasPrefix(line, "stp-priority") { | ||
| sections := strings.Fields(line) | ||
| if len(sections) > 1 { | ||
| stpPriority = sections[1] | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Most ovs-* commands have options for formatting. Are there any options you can use here to simplify the output format? Or perhaps rather than calling ovs-appctl stp/show there are ovs-vsctl commands you can use to get the value directly from the database?
There was a problem hiding this comment.
I have rewrote this function to make use of ovs-vsctl. I had avoided this approach before because the error codes returned when querying the DB for config variables do not differentiate between a bad variable name (not used) and an undefined variable.
There was a problem hiding this comment.
Thanks. Ultimately all of these functions will be migrated to use libovsdb. When that happens, MAAS will need to instantiate and use the openvswitch package differently. Even the import path will change.
lxd/network/openvswitch/ovs.go
Outdated
| stpOvsDelay = stpOvsDelay[:len(stpOvsDelay)-1] | ||
|
|
||
| stpFwDelay, err := strconv.ParseUint(stpOvsDelay, 10, 64) | ||
| return stpFwDelay * 1000, err |
There was a problem hiding this comment.
Why the multiplier? I'm guessing that the delay is reported by OVS in seconds and you would like it in milliseconds?
There was a problem hiding this comment.
That is correct, the struct api.NetworkStateBridge takes the stpFwdDelay in ms, which is why I converted it.
// Delay on port join (ms)
// Example: 1500
ForwardDelay uint64 `json:"forward_delay" yaml:"forward_delay"`
lxd/resources/network.go
Outdated
|
|
||
| bridge := api.NetworkStateBridge{} | ||
|
|
||
| if !exists { |
There was a problem hiding this comment.
exists is a confusing variable name. Can you please call it something like isOVSBridge?
lxd/resources/network.go
Outdated
| network.Bridge = &bridge | ||
|
|
There was a problem hiding this comment.
Rather than this large if/else block, can you please refactor to two functions: getOVSBridgeState and getNativeBridgeState?
lxd/state/notlinux.go
Outdated
| @@ -5,12 +5,14 @@ package state | |||
| import ( | |||
| "context" | |||
|
|
|||
| clusterConfig "github.com/canonical/lxd/lxd/cluster/config" | |||
There was a problem hiding this comment.
We don't wan't to add anything to this file that isn't absolutely necessary for the LXD agent to function. I suggest if state.State is causing issues, refactor the resources package to not require it (e.g. only pass in the fields of state.State that are actually used).
There was a problem hiding this comment.
I am not sure how feasible this is without splitting the openvswitch package in two packages.
The state.State dependency comes from importing openvswitch into the network.go file and openvswitch makes use of state.State.
The only other solution I could think off would be to move the functions in the ovs.go into their own separate package and import that into network.go, which I was already told not to do.
Let me know if you are aware of any other workarounds or how you would like me to proceed.
There was a problem hiding this comment.
Also one extra note on the scenario of splitting the openvswitch package in two. If ovs.go were to be in a separate package from ovn.go, it would still need to import the openvswitch package and result in state.State still being used because functions such as OVNBridgeMappingAdd and OVNBridgeMappings make calls to functions present in ovn.go .
This means that these functions will need to be moved back to the openvswitch package resulting in ovs related tasks being split across two packages.... which is messy.
There was a problem hiding this comment.
The only usage of state.State is where it is passed into openvswitch.NewOVN, where it access (*State).GlobalConfig. I have a PR to remove it and once merged you should be able to use the OVS utils with no issues.
|
|
Once #15084 is merged and this is rebased I'll take another look. |
State was imported only to get some config keys for setting up the OVN command connection arguments. We should try to not import state where possible, and only import the fields we need at the lowest level. This helps not only with reducing the size of imports, but also for use of packages externally, and also for unit testing. This is required for using the OVS package externally. See #15044 and #14606. cc @ghadi-rahme
Add the following OVS features: - Support for retrieving the STP priority - Generating Bridge ID - Retrieving the state of STP - Retrieving the STP forward delay - Checking if VLAN filtering is enabled - Retrieving the PVID Signed-off-by: Ghadi Elie Rahme <[email protected]>
…tion This allows OVS bridge information to be read by LXD. Other applications such as MAAS make use of this function and require the ability to retrieve OVS bridge information. Fixes canonical#14606 Signed-off-by: Ghadi Elie Rahme <[email protected]>
ghadi-rahme
force-pushed
the
adding-ovs-bridge-info
branch
from
ce0e829 to
71d20d5
Compare
|
I applied the requested changes. I also made the context a timeout context with a 5s duration for each exec function. |
…sctl, cleanups Signed-off-by: Ghadi Elie Rahme <[email protected]>
Signed-off-by: Ghadi Elie Rahme <[email protected]>
ghadi-rahme
force-pushed
the
adding-ovs-bridge-info
branch
from
71d20d5 to
530360c
Compare
Adds extra functions to ovs.go to allow for the needed OVS bridge information to be read.
These are needed for
GetNetworkState()to be able to retrieve OVS bridge information.Fixes #14606