Extract CNI networking into its own handler #26

carlosedp · 2020-01-15T17:34:00Z

Description

This refactor puts all CNI variables, constants, initialization and
functions into it's own handler.

This simplifies non-network related code like main and deploy functions
and avoids redundant code been called on every container deployment.

Signed-off-by: Carlos de Paula [email protected]

Motivation and Context

I have raised an issue to propose this change this is required

This PR replaces PR #25 and fixes issue #24.

How Has This Been Tested?

Tested on linux/amd64 deploying images integrated with faasd:

Jan 15 11:49:55 debian10 faas-containerd[9151]: 2020/01/15 11:49:55 faas-containerd starting..        Version:         Commit:         Service Timeout: 1m0s
Jan 15 11:49:55 debian10 faas-containerd[9151]: 2020/01/15 11:49:55 Writing network config...
Jan 15 11:49:55 debian10 faas-containerd[9151]: 2020/01/15 11:49:55 Listening on TCP port: 8081
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 [Update] request: {"service":"figlet9","image":"functions/figlet:0.13.0","network":"","envProcess":"figlet","envVars":{},"constraints":[],"secrets":[],"labels":{},"annotations":{},"limits":null,"requests":null,"readOnlyRootFilesystem":false}
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 [Update] service figlet9 not found
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 [Deploy] request: {"service":"figlet9","image":"functions/figlet:0.13.0","network":"","envProcess":"figlet","envVars":{},"constraints":[],"secrets":[],"labels":{},"annotations":{},"limits":null,"requests":null,"readOnlyRootFilesystem":false}
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 Deploy docker.io/functions/figlet:0.13.0 size: 5658006
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 Container ID: figlet9        Task ID figlet9:        Task PID: 9206
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 11:50:00 figlet9 has IP: 10.62.0.149.
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 16:50:00 Version: 0.13.0        SHA: fa93655d90d1518b04e7cfca7d7548d7d133a34e
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 16:50:00 Read/write timeout: 5s, 5s. Port: 8080
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 16:50:00 Writing lock-file to: /tmp/.lock
Jan 15 11:50:00 debian10 faas-containerd[9151]: 2020/01/15 16:50:00 Metrics server. Port: 8081

Client:

❯ faas store deploy figlet --name=figlet9
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

Deployed. 200 OK.
URL: http://127.0.0.1:8080/function/figlet9

❯ curl -d Test http://127.0.0.1:8081/function/figlet9
 _____         _
|_   _|__  ___| |_
  | |/ _ \/ __| __|
  | |  __/\__ \ |_
  |_|\___||___/\__|

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Commits:

I've read the CONTRIBUTION guide
My commit message has a body and describe how this was tested and why it is required.
I have signed-off my commits with git commit -s for the Developer Certificate of Origin (DCO)

Code:

My code follows the code style of this project.
I have added tests to cover my changes.

Docs:

My change requires a change to the documentation.
I have updated the documentation accordingly.

alexellis · 2020-01-15T17:37:10Z

handlers/cni_network.go

+)
+
+// defaultCNIConf is a CNI configuration that enables network access to containers (docker-bridge style)
+var defaultCNIConf = fmt.Sprintf(`


I sent a struct over to you which I think might work better. Can you take a look and see if you want to add it in this PR?

I think this might complicate things by adding Opts struct, initializing it, passing it around, etc.
Maybe later?

alexellis · 2020-01-15T17:37:51Z

handlers/cni_network.go

+`, defaultNetworkName, defaultBridgeName, defaultSubnet)
+
+// InitNetwork writes configlist file and initializes CNI network
+func InitNetwork() gocni.CNI {


It'd be a little more Go idiomatic if this could return an error and we could then throw a fatal if we wanted one level higher.

You mean returning both like last push?

handlers/cni_network.go

alexellis · 2020-01-15T17:38:24Z

handlers/cni_network.go

+		if config.Sandbox == netNamespace(task) {
+			for _, ipConfig := range config.IPConfigs {
+				if ifName != "lo" && ipConfig.IP.To4() != nil {
+					ip = ipConfig.IP


Do we not want to break here?

In case no IP is found, it's nil and returns the error following it.

It looks like we found the condition we wanted, what would happen if you broken out of the loop?

handlers/cni_network.go

alexellis

This is much clearer to read, good work. I have left some comments that I'd like you to resolve as soon as you can, then I can merge.

This refactor puts all CNI variables, constants, initialization and functions into it's own handler. This simplifies non-network related code like main and deploy functions and avoids redundant code been called on every container deployment. Signed-off-by: Carlos de Paula <[email protected]>

When deleting or updating the container, remove the created CNI network. Changed CNI id generation to be able to recover from other functions. Signed-off-by: Carlos de Paula <[email protected]>

alexellis

Approved

alexellis · 2020-01-17T19:16:31Z

I'll try this out on my other RPi3, do I need to do a custom build of the CNI plugins first?

carlosedp · 2020-01-17T19:19:08Z

Until CNI plugins release a new version (with containernetworking/plugins#434 included) the bridge plugin built from master is still needed.

alexellis · 2020-01-17T19:31:16Z

ok thanks

carlosedp mentioned this pull request Jan 15, 2020

Fix "index out of range" error while getting IP #25

Closed

11 tasks