Allow setting bond MTU #25
Conversation
|
@killianmuldoon @rkamudhan Could you please look at this one, thanks |
README.md
Outdated
| @@ -50,6 +51,7 @@ Given the following network configuration: | |||
| "type": "bond", | |||
| "mode": "active-backup", | |||
| "miimon": "100", | |||
| "mtu": 1500, | |||
There was a problem hiding this comment.
please fix the indentation here
bond/bond.go
Outdated
| @@ -42,6 +42,7 @@ type bondingConfig struct { | |||
| FailOverMac int `json:"failOverMac"` | |||
| Miimon string `json:"miimon"` | |||
| Links []map[string]interface{} `json:"links"` | |||
| MTU string `json:"mtu"` | |||
There was a problem hiding this comment.
should this be int? in the doc you mark it as int
bond/bond.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to convert bondMiimon value (%+v) to an int, error: %+v", bondMiimon, err) | ||
| } | ||
| if len(bondMTU) > 0 { |
There was a problem hiding this comment.
should we also validate that the MTU number provided is valid?
There was a problem hiding this comment.
Added validation that the mtu is not higher than the mtu of the pf of any of the vf's used. The link mac is used to match the pf's vf to the slave used in the bond.
The original vf mtu's are not relevant, as adding a link to a bond will modify it's mtu to that of the bond.
bond/bond.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to convert bondMiimon value (%+v) to an int, error: %+v", bondMiimon, err) | ||
| } | ||
| if len(bondMTU) > 0 { |
There was a problem hiding this comment.
do we also want to validate that the VFs we are going to attach have the same/higher MTU before we do it?
There was a problem hiding this comment.
The VF mtu's will be automatically changed to that of the bond.
README.md
Outdated
| @@ -31,6 +31,7 @@ The binary should be placed at /opt/cni/bin on all nodes on which bonding will t | |||
| - type (string, required): "bond" | |||
| - ifname (string, optional): name of the bond interface | |||
| - miimon (int, required): specifies the arp link monitoring frequency in milliseconds | |||
| - mtu (int, optional): the mtu of the bond | |||
There was a problem hiding this comment.
| - mtu (int, optional): the mtu of the bond | |
| - mtu (int, optional): the MTU of the bond. Default is 1500. |
This patch allows setting the MTU of the created bond. Signed-off-by: <mmirecki@redhat.com>
| if err != nil { | ||
| return fmt.Errorf("Failed to lookup physical functions links, error: %+v", err) | ||
| } | ||
| for _, pfLink := range pfLinks { |
There was a problem hiding this comment.
how you are able to get the PF info? you are inside the pod namespace already no?
There was a problem hiding this comment.
Note that a cni does not work inside a namespace. You have to enter the namespace yourself.
One of the input parameters the CNI binary is getting is the CNI_NETS environment variable, which points us to the namespace for the container it is to connect.
We are getting the netlink handle for the pf without specifying the ns (here), so we operate with no namespace here.
The slaves are retrieved in getLinkObjectsFromConfig() which takes a handle created using a ns (here).
|
/lgtm |
|
@Eoghan1232 @SchSeba thanks for the review and approval. Is there anything else missing or can this PR be merged? |
|
PR approved by myself and @SchSeba. |
Allow setting the bond MTU value.