podman network create #3862

baude · 2019-08-20T20:42:24Z

initial implementation of network create. we only support bridging
networks with this first pass.

Signed-off-by: baude [email protected]

openshift-ci-robot · 2019-08-20T20:42:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

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~~ [baude]

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

docs/podman-network-create.1.md

rhatdan · 2019-08-21T21:34:25Z

docs/podman-network-create.1.md

+Create a network with no options
+```
+# podman network create
+/etc/cni/net.d/cni-podman-4.conflist


Why not just podman-4.conflist, I don't see a vailue in prefixing this with cni, since we are already in the /etc/cni subdir

this was agreed with @mheon ...

pkg/adapter/network.go

pkg/network/config.go

pkg/network/devices.go

pkg/network/subnet.go

cmd/podman/network_create.go

completions/bash/podman

cmd/podman/cliconfig/config.go

cmd/podman/network.go

cmd/podman/network_create.go

docs/podman-network-create.1.md

pkg/adapter/network.go

pkg/network/config.go

pkg/adapter/network.go

pkg/network/config.go

mheon · 2019-08-22T21:39:11Z

pkg/network/files.go

+)
+
+// LoadCNIConfsFromDir loads all the CNI configurations from a dir
+func LoadCNIConfsFromDir(dir string) ([]*libcni.NetworkConfigList, error) {


I feel like we might want to wrap this in a sync.Once so we don't repeatedly reread the configs

?? lets talk on Monday

rhatdan · 2019-08-27T19:02:46Z

LGTM
@mheon @TomSweeneyRedHat @giuseppe @vrothberg PTAL

vrothberg

A couple of nits and comments below. Will start playing with the PR now.

cmd/podman/network_create.go

commands.md

cmd/podman/network_create.go

completions/bash/podman

pkg/network/config.go

pkg/network/devices.go

test/e2e/network_create_test.go

vrothberg · 2019-08-29T11:50:27Z

One thing I noticed is a difference in the naming scheme:

[libpod (pull-request-3862)]$ sudo ./bin/podman network create -d bridge
/etc/cni/net.d/cni-podman-0.conflist                                    

[libpod (pull-request-3862)]$ ls /etc/cni/net.d/
100-crio-bridge.conf  200-loopback.conf  87-podman-bridge.conflist  cni-podman-0.conflist

I suggest following the exisiting convention of $NUM-$TOOL-$DRIVER.conflist - unless there's a specific reason not to.

vrothberg · 2019-08-29T12:56:19Z

I tried using the generated network but it failed since it can't find a default network.

Changing the "name" from "cni-podman0" to "podman" in the generated config fixed it. Is this intended/expected?

baude

I tried using the generated network but it failed since it can't find a default network.

Changing the "name" from "cni-podman0" to "podman" in the generated config fixed it. Is this intended/expected?

the defaultnetwork needs to be documented in the conf file && actually honored. @mheon and i need to huddle on it once we merge this.

rh-atomic-bot · 2019-09-04T20:44:01Z

☔ The latest upstream changes (presumably #3876) made this pull request unmergeable. Please resolve the merge conflicts.

baude · 2019-09-08T15:53:01Z

@edsantiago do you understand whats going on with the gating failure? it was passing and i rebased and now fails.

vrothberg · 2019-09-09T07:52:15Z

@edsantiago do you understand whats going on with the gating failure? it was passing and i rebased and now fails.

cd $GOSRC && ./hack/man-page-checker |& ${TIMESTAMP}
[15:51:24] START - All [+xxxx] lines that follow are relative to right now.
[+0001s] podman-network-create.1.md        NAME= podman-network-create-
[+0002s] 
[+0002s]   podman-network-create.1.md       = 'podman\-network-create- Create a Podman CNI network'
[+0002s]   podman-network.1.md              = 'Create a CNI network for Podman'

I guess the problem is the trailing dash in podman\-network-create- . Maybe we can keep the descriptions consistent between podman-network and podman-network-create?

diff --git a/docs/podman-network-create.1.md b/docs/podman-network-create.1.md
index 45701bfc2f56..4c7cc7ef06d3 100644
--- a/docs/podman-network-create.1.md
+++ b/docs/podman-network-create.1.md
@@ -1,7 +1,7 @@
 % podman-network-create(1)
 
 ## NAME
-podman\-network-create- Create a Podman CNI network
+podman\-network\-create - Create a CNI network for Podman
 
 ## SYNOPSIS
 **podman network create**  [*options*] name

initial implementation of network create. we only support bridging networks with this first pass. Signed-off-by: baude <[email protected]>

mheon · 2019-09-09T15:46:26Z

LGTM

rhatdan · 2019-09-09T17:28:32Z

/lgtm

AkihiroSuda · 2019-09-11T08:48:03Z

Will this be available in v1.5.2? Or v1.6.0?

vrothberg · 2019-09-11T08:49:49Z

It will be available in v1.6.0. There won't be a 1.5.2.

rhatdan · 2019-09-11T12:58:05Z

@AkihiroSuda Goal is to cut 1.6.0 on Friday.

openshift-ci-robot requested review from mrunalp and TomSweeneyRedHat August 20, 2019 20:42

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 20, 2019

baude force-pushed the networkcreate branch 8 times, most recently from b51eaaf to 36f4a33 Compare August 21, 2019 15:43

rhatdan reviewed Aug 21, 2019

View reviewed changes

TomSweeneyRedHat reviewed Aug 21, 2019

View reviewed changes

cmd/podman/network_create.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Aug 21, 2019

View reviewed changes

cmd/podman/network_create.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Aug 21, 2019

View reviewed changes

cmd/podman/network_create.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Aug 21, 2019

View reviewed changes

completions/bash/podman Show resolved Hide resolved