Moving IPVLAN driver out of experimental #2230

mavenugo · 2018-07-12T04:45:52Z

IPVLAN driver had been retained in experimental for multiple releases
with the requirement to have a proper L3 control-plane (such as BGP) to
go along with it which will make this driver much more useful. But
based on the community feedback,
moby/moby#21735, am proposing to move this
driver out of experimental.

Signed-off-by: Madhu Venugopal madhu@docker.com

thaJeztah

SGTM to move out of experimental if we document the current limitations (around L3 management)

Left some comments about removing dead code

thaJeztah · 2018-07-12T07:59:50Z

drivers_experimental_linux.go

@@ -1,9 +1,5 @@
 package libnetwork

-import "github.com/docker/libnetwork/drivers/ipvlan"
-
 func additionalDrivers() []initializer {


better remove this file / function altogether now that there's currently no experimental drivers; it's not exported, so shouldn't break anyone

Perhaps best to do that in a separate commit; looks like there's quite some dead code and docs we can remove then https://github.com/docker/libnetwork/search?utf8=✓&q=experimental&type=

@thaJeztah @abhi though I agree that promoting IPVLAN driver will result in NOOP code wrt experimental drivers, I still believe it is useful to retain it instead of removing these files altogether. This acts as a template for anyone willing to add experimental drivers. I would hate to see folks spending more than necessary to add the infra to add experimental drivers.

That's what we have version control for; better to remove dead code than keeping it "just in case"

And the commit that removes it can be used as template (that's why it's good to keep it in a single, separate commit)

@thaJeztah sure. But anyone in the future who is trying to add a new experimental driver may not know this history and may not dig into the git history to identify this particular commit. They would have spent valuable time redoing the work and we may have to point them back to this commit, etc. Having framework code in place is useful in certain situations.

IMO Code that performs a logic or functional in nature being dead will confuse everyone, while framework code will help guide folks. In any case, if other maintainers prefer to remove this unused code (which it seems like), I will go ahead and remove it.

But anyone in the future who is trying to add a new experimental driver may not know this history and may not dig into the git history to identify this particular commit.

Shouldn't someone who is adding a new new experimental driver be talking to the community up front? If so then at that point they can be pointed at the history/commit. If not, well, that's why we suggest that people discuss big things before doing lots of work...

thaJeztah · 2018-07-12T08:00:42Z

drivers_linux.go

@@ -14,6 +15,7 @@ func getInitializers(experimental bool) []initializer {
 		{bridge.Init, "bridge"},
 		{host.Init, "host"},
 		{macvlan.Init, "macvlan"},
+		{ipvlan.Init, "ipvlan"},


Is this list sorted alphabetically? 😇

selansen · 2018-07-14T03:07:19Z

LGTM

abhi · 2018-07-16T20:36:19Z

drivers_linux.go

@@ -14,6 +15,7 @@ func getInitializers(experimental bool) []initializer {
 		{bridge.Init, "bridge"},
 		{host.Init, "host"},
 		{macvlan.Init, "macvlan"},
+		{ipvlan.Init, "ipvlan"},


abhi · 2018-07-16T20:36:27Z

drivers_experimental_linux.go

@@ -1,9 +1,5 @@
 package libnetwork

-import "github.com/docker/libnetwork/drivers/ipvlan"
-
 func additionalDrivers() []initializer {


IPVLAN driver had been retained in experimental for multiple releases with the requirement to have a proper L3 control-plane (such as BGP) to go along with it which will make this driver much more useful. But based on the community feedback, moby/moby#21735, am proposing to move this driver out of experimental. Signed-off-by: Madhu Venugopal <madhu@docker.com>

Signed-off-by: Madhu Venugopal <madhu@docker.com>

mavenugo · 2018-09-25T20:42:36Z

@thaJeztah @ijc addressed the comments. PTAL.

thaJeztah

LGTM

thaJeztah · 2018-09-26T13:06:27Z

Not a maintainer here, so can't merge 😅

thaJeztah · 2018-10-30T08:53:52Z

ping @fcrisciani PTAL

Antauri · 2018-11-06T20:22:31Z

Glad to see some action here! We are watching this so closely. 1y or more in production on IPVLAN (with the limitations managed by ourselves with an Ansible-based abstraction layer). If this gets moved out of experimental we're gonna pop the beers.

Hopefully the Kubernetes guys also pick it up, making it a viable option to take in consideration on our side (we didn't choose experimenting with Kubernetes since information on setting-up Kubernetes + IPVLAN is lacking and we required IPVLAN for the network performance and isolation reasons we've outlined in the original feedback).

We want get drunk, so if there's a maintainer here, we'll drink in honor of your name if you could be kind enough to approve and capture this in an upcoming release. 👍 🍻

full diff: moby/libnetwork@1a06131...ebcade7 relevant changes: - moby/libnetwork#2349 IPVS: Add support for GetConfig/SetConfig - moby/libnetwork#2343 Revert "debian has iptables-legacy and iptables-nft now" - moby/libnetwork#2230 Moving IPVLAN driver out of experimental - moby/libnetwork#2307 Fix for problem where agent is stopped and does not restart - moby/libnetwork#2303 Touch-up error-message and godoc for ConfigVXLANUDPPort - moby/libnetwork#2325 Fix possible nil pointer exception - moby/libnetwork#2302 Use sync.RWMutex for VXLANUDPPort - moby/libnetwork#2306 Improve error if auto-selecting IP-range failed Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: moby/libnetwork@1a06131...ebcade7 relevant changes: - moby/libnetwork#2349 IPVS: Add support for GetConfig/SetConfig - moby/libnetwork#2343 Revert "debian has iptables-legacy and iptables-nft now" - moby/libnetwork#2230 Moving IPVLAN driver out of experimental - moby/libnetwork#2307 Fix for problem where agent is stopped and does not restart - moby/libnetwork#2303 Touch-up error-message and godoc for ConfigVXLANUDPPort - moby/libnetwork#2325 Fix possible nil pointer exception - moby/libnetwork#2302 Use sync.RWMutex for VXLANUDPPort - moby/libnetwork#2306 Improve error if auto-selecting IP-range failed Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 3ab093d5670e8d59f6ae0c4604b8fcabf1582854 Component: engine

full diff: moby/libnetwork@1a06131...ebcade7 relevant changes: - moby/libnetwork#2349 IPVS: Add support for GetConfig/SetConfig - moby/libnetwork#2343 Revert "debian has iptables-legacy and iptables-nft now" - moby/libnetwork#2230 Moving IPVLAN driver out of experimental - moby/libnetwork#2307 Fix for problem where agent is stopped and does not restart - moby/libnetwork#2303 Touch-up error-message and godoc for ConfigVXLANUDPPort - moby/libnetwork#2325 Fix possible nil pointer exception - moby/libnetwork#2302 Use sync.RWMutex for VXLANUDPPort - moby/libnetwork#2306 Improve error if auto-selecting IP-range failed Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

mavenugo force-pushed the exp-ipv branch from 6a11182 to 01f84d5 Compare July 12, 2018 05:39

rreinurm mentioned this pull request Jul 12, 2018

Consider moving ipvlan network driver out of experimental moby/moby#35767

Open

thaJeztah requested changes Jul 12, 2018

View reviewed changes

abhi approved these changes Jul 16, 2018

View reviewed changes

mavenugo force-pushed the exp-ipv branch from 01f84d5 to 69ed65f Compare July 20, 2018 04:02

mavenugo added 2 commits September 25, 2018 10:30

Removing experimental driver interface

388a43b

Signed-off-by: Madhu Venugopal <madhu@docker.com>

mavenugo force-pushed the exp-ipv branch from 69ed65f to 388a43b Compare September 25, 2018 17:31

ijc approved these changes Sep 26, 2018

View reviewed changes

thaJeztah approved these changes Sep 26, 2018

View reviewed changes

fcrisciani merged commit 8be2d07 into moby:master Mar 1, 2019

thaJeztah mentioned this pull request Apr 1, 2019

bump libnetwork to ebcade70ad1059b070d0040d798ecca359bc5fed moby/moby#38983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving IPVLAN driver out of experimental #2230

Moving IPVLAN driver out of experimental #2230

mavenugo commented Jul 12, 2018

thaJeztah left a comment

thaJeztah Jul 12, 2018

thaJeztah Jul 12, 2018

abhi Jul 16, 2018

mavenugo Jul 19, 2018

thaJeztah Jul 19, 2018

thaJeztah Jul 19, 2018

mavenugo Jul 27, 2018

ijc Jul 27, 2018

thaJeztah Jul 12, 2018

abhi Jul 16, 2018

selansen commented Jul 14, 2018

abhi Jul 16, 2018

abhi Jul 16, 2018

mavenugo commented Sep 25, 2018

thaJeztah left a comment

thaJeztah commented Sep 26, 2018

thaJeztah commented Oct 30, 2018

Antauri commented Nov 6, 2018

Moving IPVLAN driver out of experimental #2230

Moving IPVLAN driver out of experimental #2230

Conversation

mavenugo commented Jul 12, 2018

thaJeztah left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selansen commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Sep 25, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Sep 26, 2018

thaJeztah commented Oct 30, 2018

Antauri commented Nov 6, 2018