hotplug feature: add pod networks annotation controller #777

maiqueb · 2022-01-13T15:22:56Z

Features:

a very opinionated refactor of multus, allowing its delegate handling functions to be called outside the package
a controller listening to multus annotations. When the multus annotations changes, it will invoke the corresponding delegate
e2e tests
two runtimes (required to read to network namespace of the pod being updated - which is a CNI parameter)
- containerd
- crio
unit tests of every new package

TODO:

figure out what is holding this PR
act upon revive's comments

maiqueb · 2022-01-13T16:16:08Z

This forced push features:

removed the status condition updates
fixed revive's comments (causing the unit test jobs to fail).

coveralls · 2022-01-13T16:18:00Z

Pull Request Test Coverage Report for Build 1993977572

59 of 165 (35.76%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.3%) to 64.81%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/types/conf.go	0	3	0.0%
pkg/k8sclient/k8sclient.go	16	21	76.19%
pkg/multus/multus.go	39	52	75.0%
pkg/types/extractors.go	0	85	0.0%

Totals
Change from base Build 1952559576:	-2.3%
Covered Lines:	1722
Relevant Lines:	2657

💛 - Coveralls

maiqueb · 2022-01-13T17:37:52Z

@s1061123 I think the approach you shared offline is in line w/ this commit: df801b2

I would appreciate if you'd help me understand how to build the missing data on the controller.

Furthermore, a logic split is required on the multus CmdAdd function.

If we take this route, when the time comes to handle the default route update, another serious refactor on the multus pkg will be required.

Multus is refactored as a thick plugin, featuring 2 main components: - a server listening to a unix domain socket, running in a pod - a shim, a binary on the host that will send JSON requests built from its environment / stdin values to the aforementioned server. The pod where the multus daemon is running must share the host's PID namespace. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

Without this patch, we're blindly trusting anything sent by the server. This way, we assure the requests arriving at the multus controller are valid before hand. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Also add a new command line parameter on the multus controller, pointing it to the server configuration. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

CNI is already filling the args structure; we should consume that rather than rely on the environment variables. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

Refactor multus as a thick plugin

setenv refers environment variables, which is unique in process, not unique to go routine. Hence it may causes some issue in multi threaded case, hence it is replaced with libcni's runtimeConfig value set to set these variables at libcni side, after process fork.

This change make binary file and directory name consistent. In addition, change the package name cni to server because cni is a bit umbiguous for cni plugin's repository.

To simplify multus unit tests, split it into several files, based on testing CNI version.

…lumbingwg#798) * multus: entrypoint: disallow incompatible cni versions When top level CNI version is 0.4.0 or more, nested CNI version can't be less than 0.4.0 since these are incompatible. This closes issue k8snetworkplumbingwg#737. Signed-off-by: Balazs Nemeth <[email protected]> * multus: thick: disallow incompatible cni versions Similarly to disallowing incompatible versions in entrypoint.sh, add the same logic in go for the thick plugin. Signed-off-by: Balazs Nemeth <[email protected]> * multus: add unit test for incompatible cni versions Signed-off-by: Balazs Nemeth <[email protected]> Co-authored-by: Balazs Nemeth <[email protected]>

…multus_ut Split multus unit tests into several files

Fix k8snetworkplumbingwg#792

Support CNI 1.0.0

Signed-off-by: Miguel Duarte Barroso <[email protected]>

A follow-up PR will add the cri-o runtime. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

Introduce tests to check: - hotplug a new interface - remove an old interface Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit defines the controller constructor: - use informer factories instead of passing the client - inject the container runtime into the controller Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

By using the queue, we can re-queue when the request fails, which makes the solution more robust. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb · 2022-04-21T08:16:48Z

pkg/containerruntimes/crio/crio_test.go

+			runtime = newDummyCrioRuntime(fake.WithCachedContainer(containerID, netnsPath))
+		})
+
+		It("cannot extract the network namespace of a container", func() {


the it clause description is wrong, probably a copy paste error.

martinkennelly · 2022-05-26T16:07:24Z

/assign @martinkennelly

maiqueb · 2022-08-24T15:56:28Z

This is being implemented in https://github.com/maiqueb/multus-dynamic-networks-controller/, now that a 3rd party controller with access to the multus thick-plugin socket can trigger delegate add / delete.

maiqueb force-pushed the add-networks-annotation-controller branch from 2434098 to f99f08e Compare January 13, 2022 16:13

maiqueb force-pushed the add-networks-annotation-controller branch 2 times, most recently from c99daa7 to df801b2 Compare January 13, 2022 17:35

maiqueb force-pushed the add-networks-annotation-controller branch 3 times, most recently from 249a538 to 3d4cb6d Compare January 14, 2022 13:39

maiqueb added 10 commits February 9, 2022 11:43

react to maintainers review

bba9474

Signed-off-by: Miguel Duarte Barroso <[email protected]>

thick, deployment: update the daemonset spec

aed83d5

Signed-off-by: Miguel Duarte Barroso <[email protected]>

thick, config: validate the cni config passed by the runtime

c163cc7

Without this patch, we're blindly trusting anything sent by the server. This way, we assure the requests arriving at the multus controller are valid before hand. Signed-off-by: Miguel Duarte Barroso <[email protected]>

thick: model client / server config

6e22328

Also add a new command line parameter on the multus controller, pointing it to the server configuration. Signed-off-by: Miguel Duarte Barroso <[email protected]>

SQUASH candidate, thick, config: cleanup the configuration

3f31f0d

Signed-off-by: Miguel Duarte Barroso <[email protected]>

multus: use args.args instead of an env variable

41ce328

CNI is already filling the args structure; we should consume that rather than rely on the environment variables. Signed-off-by: Miguel Duarte Barroso <[email protected]>

unit tests: remove weird tests that check an impossible scenario

ddcd865

Signed-off-by: Miguel Duarte Barroso <[email protected]>

docs, thick: document the thick plugin variant

7fcfe77

Signed-off-by: Miguel Duarte Barroso <[email protected]>

thick, server, multus: re-use common types

1d228ee

Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb changed the title ~~hotplug feature: add pod networks annotation controller~~ WIP: hotplug feature: add pod networks annotation controller Feb 17, 2022

dougbtv and others added 10 commits February 21, 2022 09:13

Merge pull request k8snetworkplumbingwg#771 from maiqueb/thick-arch

b800e09

Refactor multus as a thick plugin

Make binary file and directory name consistent

b652f86

This change make binary file and directory name consistent. In addition, change the package name cni to server because cni is a bit umbiguous for cni plugin's repository.

Simplify e2e scripts (k8snetworkplumbingwg#795)

20224de

Split multus unit tests into several files

e6e854a

To simplify multus unit tests, split it into several files, based on testing CNI version.

Merge pull request k8snetworkplumbingwg#797 from s1061123/dev/refine_…

5b3f05e

…multus_ut Split multus unit tests into several files

Support CNI 1.0.0

107121c

Fix k8snetworkplumbingwg#792

Merge pull request k8snetworkplumbingwg#800 from s1061123/dev/cni100

c7be533

Support CNI 1.0.0

Fix thick plugin to run kind-e2e test

0864e88

maiqueb changed the base branch from master to feature/multus-4.0 March 14, 2022 11:28

maiqueb force-pushed the add-networks-annotation-controller branch 5 times, most recently from d742b2e to 7c8c099 Compare March 16, 2022 16:11

maiqueb mentioned this pull request Mar 25, 2022

multus cni: add / remove single network / interface #752

Closed

maiqueb added 14 commits March 28, 2022 17:47

controller: listen to pod network annotation updates

49d49d6

Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller, containerd: add CRI runtime

2dbaef1

A follow-up PR will add the cri-o runtime. Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller, cni: invoke cni API add/remove network

ed66763

Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller, crio: add new runtime

da7adbe

Signed-off-by: Miguel Duarte Barroso <[email protected]>

yaml templating: remove unnecessary config

544418b

Signed-off-by: Miguel Duarte Barroso <[email protected]>

e2e, tests: re-write macvlan tests in golang

1e06a12

Signed-off-by: Miguel Duarte Barroso <[email protected]>

e2e tests, hotplug: test hotplug interface

c7e9da6

Introduce tests to check: - hotplug a new interface - remove an old interface Signed-off-by: Miguel Duarte Barroso <[email protected]>

ci: only run golang e2e on the thick img variant

1dd0059

Signed-off-by: Miguel Duarte Barroso <[email protected]>

config, cri: specify the cri in the multus server config

9641f7c

Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller: user informer factories

297d6ed

This commit defines the controller constructor: - use informer factories instead of passing the client - inject the container runtime into the controller Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller: use net-attach-def informer & lister

62f899c

Signed-off-by: Miguel Duarte Barroso <[email protected]>

controller: use a workqueue, to provide a level trigger controller

1728706

By using the queue, we can re-queue when the request fails, which makes the solution more robust. Signed-off-by: Miguel Duarte Barroso <[email protected]>

containerd, unit tests: add unit tests to containerd runtime

a6dc413

Signed-off-by: Miguel Duarte Barroso <[email protected]>

crio, unit tests: add unit tests to cri-o runtime

a8ce32c

Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb force-pushed the add-networks-annotation-controller branch from 7c8c099 to a8ce32c Compare March 28, 2022 16:00

maiqueb changed the title ~~WIP: hotplug feature: add pod networks annotation controller~~ hotplug feature: add pod networks annotation controller Mar 28, 2022

maiqueb mentioned this pull request Mar 30, 2022

Support nic hotplug kubevirt/kubevirt#6733

Closed

s1061123 force-pushed the feature/multus-4.0 branch 2 times, most recently from 64b08ca to bf4d6c7 Compare April 12, 2022 12:42

maiqueb commented Apr 21, 2022

View reviewed changes

maiqueb closed this Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hotplug feature: add pod networks annotation controller #777

hotplug feature: add pod networks annotation controller #777

maiqueb commented Jan 13, 2022 •

edited

Loading

maiqueb commented Jan 13, 2022

coveralls commented Jan 13, 2022 •

edited

Loading

maiqueb commented Jan 13, 2022

maiqueb Apr 21, 2022

martinkennelly commented May 26, 2022

maiqueb commented Aug 24, 2022

hotplug feature: add pod networks annotation controller #777

hotplug feature: add pod networks annotation controller #777

Conversation

maiqueb commented Jan 13, 2022 • edited Loading

maiqueb commented Jan 13, 2022

coveralls commented Jan 13, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1993977572

💛 - Coveralls

maiqueb commented Jan 13, 2022

maiqueb Apr 21, 2022

Choose a reason for hiding this comment

martinkennelly commented May 26, 2022

maiqueb commented Aug 24, 2022

maiqueb commented Jan 13, 2022 •

edited

Loading

coveralls commented Jan 13, 2022 •

edited

Loading