Support docker-specific network create options via CLI #1320
Conversation
hasan4791
changed the title
|
LGTM |
hasan4791
force-pushed
the
issue-15830
branch
from
69e4db7 to
28a1f48
Compare
|
I'm not sure why this is failing, even though both the actual & expected is equal |
libnetwork/netavark/config.go
Outdated
| err = internalutil.CreateBridge(n, newNetwork, usedNetworks, n.defaultsubnetPools) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // validate the given options, we do not need them but just check to make sure they are valid |
There was a problem hiding this comment.
I assume this comment is outdated? The code after it used to just parse the options as-is, and stop if an error occurred. But now we are actually adapting the options as necessary.
There was a problem hiding this comment.
Thank you @dpward , addressed the same.
|
/approve |
|
For Doc update, |
|
@hasan4791 Please combine the two PRs together into this PR and fix @dpward comment #1320 (review) and then we can merge. |
|
@rhatdan But the doc update PR is on podman repo and it can be merged when we update the containers/common version. |
hasan4791
force-pushed
the
issue-15830
branch
from
28a1f48 to
7a7af11
Compare
|
@hasan4791 Thanks I missed that. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hasan4791, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
I think this should be put in a separate function which maps the docker key to the podman keys one to one and then removes the docker key from the map. I suggest you put this in libnetwork/util
Right now this completely ignores the cni backend and drivers other than bridge, if you move this into the separate package you can just import it in both the netavark and cni code and call it before the driver switch case.
libnetwork/netavark/config.go
Outdated
| case "com.docker.network.driver.mtu": | ||
| newNetwork.Options[types.MTUOption] = value | ||
| fallthrough | ||
|
|
There was a problem hiding this comment.
you should to remove the docker option from the map otherwise we end up with two times the same option in the config which is undesirable.
There was a problem hiding this comment.
these docker options are applicable only for bridge drivers right?
There was a problem hiding this comment.
Addressed review comments. PTAL
There was a problem hiding this comment.
I've added only to the bridge driver option as per this doc
https://docs.docker.com/engine/reference/commandline/network_create/#bridge-driver-options
hasan4791
force-pushed
the
issue-15830
branch
from
7a7af11 to
6750e4a
Compare
Signed-off-by: T K Chandra Hasan <t.k.chandra.hasan@ibm.com>
hasan4791
force-pushed
the
issue-15830
branch
from
6750e4a to
790e87b
Compare
|
This particular testcase is randomly failing, "return the same network when creating two networks with the same name and ignore" |
|
/lgtm |
Signed-off-by: T K Chandra Hasan t.k.chandra.hasan@ibm.com
Following docker specific network options are supported via CLI
Fixes containers/podman#15830