Add podman network create flag for bridge mtu #8457

afbjorklund · 2020-11-23T21:08:11Z

See #8454

rhatdan · 2020-11-23T21:22:05Z

You need to update man pages

rhatdan · 2020-11-23T21:22:52Z

Need tests, and I don't think this will work on podman-remote right now.

rhatdan · 2020-11-23T21:24:22Z

@baude @mheon PTAL

mheon · 2020-11-23T21:32:03Z

cmd/podman/networks/create.go

@@ -42,6 +42,8 @@ func networkCreateFlags(cmd *cobra.Command) {

 	flags.BoolVar(&networkCreateOptions.Internal, "internal", false, "restrict external access from this network")

+	flags.IntVar(&networkCreateOptions.MTU, "mtu", 0, "MTU")


Should we do a more general options flag? It seems like there could be quite a lot of these.

(The Docker extremely-long-option format is probably unnecessary, but something like --bridge-option mtu=8000?

I suppose there couldn't be many more than what the CNI "bridge" exposes, but hopefully the user (OP) has more input...

In my use case I need only the MTU setting to sync it with an underlying network.
When looking at the bridge driver parameters, there's maybe one more good candidate for this - the vlan tag parameter..

I agree with @mheon Lets do this as --bridge-option and just add the two for now, which leaves us room for more in the future without exploding out CLI.

The other commands use "opt" (instead of option), for instance the one from "podman volume create"

-o, --opt stringArray Set driver specific options (default [])

Like the ones from "run":

--dns-opt strings Set custom DNS options --log-opt strings Logging driver options --security-opt stringArray Security Options

TomSweeneyRedHat · 2020-11-23T22:53:41Z

docs/source/markdown/podman-network-create.1.md

@@ -45,6 +45,10 @@ must be used with a *subnet* option.
 Create a *Macvlan* based connection rather than a classic bridge.  You must pass an interface name from the host for the
 Macvlan connection.

+#### **--mtu**
+
+The MTU (integer, optional).


Chunked this together form Wikipedia, is this correct or needs adjusting? I think we need a bit more info than what's here at the moment.

Suggested change

The MTU (integer, optional).

The Maximum Transmission Unit (MTU), the size of the largest protocol data unit that can be communicated in a single network-layer transaction (integer, optional).

Hopefully the middle road is OK, with the new options

afbjorklund · 2020-11-24T18:59:32Z

Need tests, and I don't think this will work on podman-remote right now.

I will need some help with this (integration test, and podman remote support)

rhatdan · 2020-11-24T19:45:27Z

@afbjorklund When you get this to a state that you are happy with, we can get someone to take it over if you want.

TomSweeneyRedHat · 2020-11-24T19:49:11Z

docs/source/markdown/podman-network-create.1.md

+Set driver specific options.
+
+For the `bridge` driver the following options are supported: `mtu`.
+The `mtu` option sets the Maximum Transmission Unit (MTU) and takes an integer value.


afbjorklund · 2020-11-24T20:15:37Z

I think this should work, and the flag matches the "network" which also uses drivers.

Usage:
  podman volume create [options] [NAME]

Options:
      --driver string     Specify volume driver name (default "local")
  -l, --label strings     Set metadata for a volume (default [])
  -o, --opt stringArray   Set driver specific options (default [])

Usage:
  podman network create [options] [NETWORK]

Options:
  -d, --driver string     driver to manage the network (default "bridge")
  -o, --opt stringArray   Set driver specific options (default [])

And then we don't need another option, for the second driver ?

Was there another flag that should be added as well ? I think "vlan" was mentioned

https://www.cni.dev/plugins/main/bridge/

forceAddress (boolean, optional): Indicates if a new IP address should be set if the previous value has been changed. Defaults to false.
mtu (integer, optional): explicitly set MTU to the specified value. Defaults to the value chosen by the kernel.
promiscMode (boolean, optional): set promiscuous mode on the bridge. Defaults to false.
vlan (int, optional): assign VLAN tag. Defaults to none.

The remaining feature is --label, but as mentioned that needs some more work:

#7989 (comment)

Minikube is using labels, to identify which volumes and networks are OK to remove...

--filter='label=created_by.minikube.sigs.k8s.io=true'

The workaround is for the user to clean them up manually...

Volume does have --label, but the --filter was missing value. (~~#8341~~)

Luap99

podman-remote works since we are using the full NetworkCreateOptions struct as json in the body.

For the integration tests I recommend adding this to test/e2e/network_create_test.go:

It("podman network create with mtu option", func() {
	net := "mtu-test"
	nc := podmanTest.Podman([]string{"network", "create", "--opt", "mtu=9000", net})
	nc.WaitWithDefaultTimeout()
	Expect(nc.ExitCode()).To(BeZero())
	defer podmanTest.removeCNINetwork(net)

	nc = podmanTest.Podman([]string{"network", "inspect", net})
	nc.WaitWithDefaultTimeout()
	Expect(nc.ExitCode()).To(BeZero())
	Expect(nc.OutputToString()).To(ContainSubstring(`"mtu": 9000,`))
})

cmd/podman/networks/create.go

libpod/network/create.go

Luap99

LGTM

TomSweeneyRedHat · 2020-11-30T18:15:57Z

docs/source/markdown/podman-network-create.1.md

+
+For the `bridge` driver the following options are supported: `mtu` and `vlan`.
+The `mtu` option sets the Maximum Transmission Unit (MTU) and takes an integer value.
+The `vlan` option assign VLAN tag and enables vlan\_filtering. Defaults to none.


Just triple checking, I think the '` is intentional and needed?

Sorry which one ? The backslash ? Editor was complaining about the "naked" underscore, so I escaped it... Maybe needlessly so. Not sure if it was seen as "half an italic" or something

oops, messed up the md prior. vlan\_filtering is what I was asking about. Seems to resolve OK, I'm just not sure it's necessary.

TomSweeneyRedHat · 2020-11-30T18:16:26Z

LGTM

zhangguanzhang · 2020-12-01T01:02:26Z

may be need to rebase to 1 commit?

rhatdan · 2020-12-01T20:44:34Z

@afbjorklund needs a rebase, and then we can get this in.

Signed-off-by: Anders F Björklund <[email protected]>

Thanks Luap99 for doing the implementation Signed-off-by: Anders F Björklund <[email protected]>

Thanks Luap99 for the validation suggestion Signed-off-by: Anders F Björklund <[email protected]>

rhatdan · 2020-12-01T23:59:46Z

/approve
/lgtm

openshift-ci-robot · 2020-12-01T23:59:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, 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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon reviewed Nov 23, 2020

View reviewed changes

afbjorklund force-pushed the bridge-mtu branch from ffd4722 to e9949fd Compare November 23, 2020 22:21

TomSweeneyRedHat reviewed Nov 23, 2020

View reviewed changes

rhatdan added 3.0 Features kind/feature Categorizes issue or PR as related to a new feature. labels Nov 24, 2020

afbjorklund force-pushed the bridge-mtu branch from e9949fd to 3c836ec Compare November 24, 2020 18:59

TomSweeneyRedHat reviewed Nov 24, 2020

View reviewed changes

Luap99 reviewed Nov 24, 2020

View reviewed changes

cmd/podman/networks/create.go Outdated Show resolved Hide resolved

libpod/network/create.go Show resolved Hide resolved

libpod/network/create.go Outdated Show resolved Hide resolved

Luap99 mentioned this pull request Nov 25, 2020

Support dnsname plugin in networks created with internal flag (support isDefaultGateway) #8483

Closed

afbjorklund force-pushed the bridge-mtu branch from 3c836ec to e8eb15c Compare November 26, 2020 16:51

Luap99 reviewed Nov 26, 2020

View reviewed changes

afbjorklund mentioned this pull request Nov 29, 2020

Add private network implementation for podman kubernetes/minikube#9716

Merged

TomSweeneyRedHat reviewed Nov 30, 2020

View reviewed changes

afbjorklund added 4 commits December 1, 2020 22:33

Add podman network create option for bridge mtu

b1b3570

Signed-off-by: Anders F Björklund <[email protected]>

Add podman network create option for bridge vlan

7f1be76

Signed-off-by: Anders F Björklund <[email protected]>

Add integration test for the bridge options

de2b15f

Thanks Luap99 for doing the implementation Signed-off-by: Anders F Björklund <[email protected]>

Validate that the bridge option is supported

db70e91

Thanks Luap99 for the validation suggestion Signed-off-by: Anders F Björklund <[email protected]>

afbjorklund force-pushed the bridge-mtu branch from 9bbc3e3 to db70e91 Compare December 1, 2020 21:34

openshift-ci-robot assigned rhatdan Dec 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020

openshift-merge-robot merged commit c585012 into containers:master Dec 2, 2020

This was referenced Dec 2, 2020

podman network create: add option to set custom network driver settings, eg like MTU #8454

Closed

add support for setting MTU in podman_network containers/ansible-podman-collections#147

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add podman network create flag for bridge mtu #8457

Add podman network create flag for bridge mtu #8457

afbjorklund commented Nov 23, 2020

rhatdan commented Nov 23, 2020

rhatdan commented Nov 23, 2020

rhatdan commented Nov 23, 2020

mheon Nov 23, 2020

mheon Nov 23, 2020

afbjorklund Nov 23, 2020

pschiffe Nov 23, 2020

rhatdan Nov 24, 2020

afbjorklund Nov 24, 2020

rhatdan Nov 24, 2020

TomSweeneyRedHat Nov 23, 2020

afbjorklund Nov 24, 2020

afbjorklund commented Nov 24, 2020

rhatdan commented Nov 24, 2020

TomSweeneyRedHat Nov 24, 2020

afbjorklund commented Nov 24, 2020

Luap99 left a comment

Luap99 left a comment

TomSweeneyRedHat Nov 30, 2020

afbjorklund Nov 30, 2020 •

edited

Loading

TomSweeneyRedHat Nov 30, 2020

TomSweeneyRedHat commented Nov 30, 2020

zhangguanzhang commented Dec 1, 2020

rhatdan commented Dec 1, 2020

rhatdan commented Dec 1, 2020

openshift-ci-robot commented Dec 1, 2020

		@@ -42,6 +42,8 @@ func networkCreateFlags(cmd *cobra.Command) {

		flags.BoolVar(&networkCreateOptions.Internal, "internal", false, "restrict external access from this network")

		flags.IntVar(&networkCreateOptions.MTU, "mtu", 0, "MTU")

	The MTU (integer, optional).
	The Maximum Transmission Unit (MTU), the size of the largest protocol data unit that can be communicated in a single network-layer transaction (integer, optional).

Add podman network create flag for bridge mtu #8457

Add podman network create flag for bridge mtu #8457

Conversation

afbjorklund commented Nov 23, 2020

rhatdan commented Nov 23, 2020

rhatdan commented Nov 23, 2020

rhatdan commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afbjorklund commented Nov 24, 2020

rhatdan commented Nov 24, 2020

Choose a reason for hiding this comment

afbjorklund commented Nov 24, 2020

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afbjorklund Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Nov 30, 2020

zhangguanzhang commented Dec 1, 2020

rhatdan commented Dec 1, 2020

rhatdan commented Dec 1, 2020

openshift-ci-robot commented Dec 1, 2020

afbjorklund Nov 30, 2020 •

edited

Loading