Integrate macvtap cni #346

maiqueb · 2020-03-12T16:21:13Z

Add macvtap CNI to the list of network addons installed by the CNAO operator.

It deploys and empty configuration, which currently doesn't expose any host interfaces.

Once kubevirt/macvtap-cni#11 is merged, it would start to expose all the host's interfaces.

Add macvtap-cni as a network addon.

maiqueb · 2020-03-12T16:50:14Z

/cc @phoracek

qinqon · 2020-03-13T13:52:16Z

tools/manifest-templator/manifest-templator.go

@@ -55,7 +55,7 @@ type templateData struct {
 	OperatorVersion string
 	Namespace       string
 	ContainerPrefix string
-	ImageName	string
+	ImageName	    string
 	ContainerTag    string


go fmt is good w/ it ...

EDIT: my bad, commented on outdated code; please confirm it's OK now.

tools/manifest-templator/manifest-templator.go

phoracek

Please add a new section to the README. Explain how to make it expose some ifaces.

data/macvtap/001-rbac.yaml

phoracek · 2020-03-13T15:26:13Z

data/macvtap/002-macvtap-daemonset.yaml

@@ -0,0 +1,63 @@
+---
+kind: ConfigMap


Please try to edit this CM deployed by CNAO. I have a bad feeling the settings would get reverted by the operator. e.g. deploy macvtap, edit the CM, additionally deploy ovs, see if the CM remained in the original state.

If the CM changes, we have two options:

Ask user to deploy it themselves

Add explicit setting to CNAO not to touch it

I feel the former is better.

Yup, after updating the value, the operator puts it back the way it was.

Remember that in kubevirt/macvtap-cni#11 the configmap is still required, and that the macvtap DS won't come up until the config map is available.

That would require us (at least in the tests) to create the config map by some other means (hacky move).

How can we add the explicit setting to CNAO to stay away from the config map ? What are the disadvantages of this option ?

Disadvantage is that we spoil the generic code with "if macvtap DP".

I was writing a guide on "how to hack it", but I think there is an elegant way too.

So this issue where we want to keep user changes on objects deployed is pretty common, although we were successful avoiding addressing it, until now.

So let's think how can we make it generic in the CNAO framework and keep everything specific to the component under data/ or pkg/network.

Option A: The simplest fit into the current API would be to introduce a Kubernetes object label networkaddonsoperator.network.kubevirt.io/untouchable: "true" (or any better name after /). That can be set by a user who wants to debug manual changes on any deployed component (e.g. they want to change resource requests, add debugging flags to running apps etc). It can be also set by default under data/ manifests. The only code we need to introduce then would live around https://github.com/kubevirt/cluster-network-addons-operator/blob/master/pkg/apply/apply.go#L37, where we check if an existing object we want to update has this flag, if it does, we ignore it. If you implement it, please do it in a separate PR (in case we need to backport it) and document it at the bottom of the README.

I am tempted to say "let's just keep it as it is and wait for the proper API", but that would make it harder to test under kubevirt and if somebody unaware uses it (despite it us "experimental", they may get hurt).

data/macvtap/002-macvtap-daemonset.yaml

test/e2e/workflow/validation_test.go

test/check/components.go

pkg/network/macvtap.go

maiqueb

Will push a new version w/out the RBAC.

maiqueb · 2020-03-13T16:32:04Z

data/macvtap/002-macvtap-daemonset.yaml

@@ -0,0 +1,63 @@
+---
+kind: ConfigMap


Yup, after updating the value, the operator puts it back the way it was.

Remember that in kubevirt/macvtap-cni#11 the configmap is still required, and that the macvtap DS won't come up until the config map is available.

That would require us (at least in the tests) to create the config map by some other means (hacky move).

How can we add the explicit setting to CNAO to stay away from the config map ? What are the disadvantages of this option ?

data/macvtap/002-macvtap-daemonset.yaml

pkg/network/macvtap.go

test/check/components.go

test/e2e/workflow/validation_test.go

maiqueb · 2020-03-13T16:44:23Z

tools/manifest-templator/manifest-templator.go

@@ -55,7 +55,7 @@ type templateData struct {
 	OperatorVersion string
 	Namespace       string
 	ContainerPrefix string
-	ImageName	string
+	ImageName	    string
 	ContainerTag    string


go fmt is good w/ it ...

EDIT: my bad, commented on outdated code; please confirm it's OK now.

data/macvtap/001-rbac.yaml

maiqueb · 2020-03-13T18:32:42Z

/retest

maiqueb · 2020-03-13T18:39:19Z

README.md

+Macvtap-cni must be explicitly configured by the administrator, indicating the
+interfaces on top of which logical networks can be created.
+
+A simple example on how to do so, the user must deploy a `ConfigMap`, such as in [this example](https://github.com/kubevirt/macvtap-cni/blob/master/examples/macvtap-deviceplugin-config.yaml).


Should we set this example in https://github.com/kubevirt/cluster-network-addons-operator/pull/346/files#diff-e1a65158257c165bb0c41bf08016e58bR2-R8 @phoracek ?

Currently we don't have any tests of functionality of operands. I really want to add at least basic sanity checks, but we need to setup test lane for it first. No need to add the test.

phoracek · 2020-03-13T21:29:50Z

README.md

+Macvtap-cni must be explicitly configured by the administrator, indicating the
+interfaces on top of which logical networks can be created.
+
+A simple example on how to do so, the user must deploy a `ConfigMap`, such as in [this example](https://github.com/kubevirt/macvtap-cni/blob/master/examples/macvtap-deviceplugin-config.yaml).


Currently we don't have any tests of functionality of operands. I really want to add at least basic sanity checks, but we need to setup test lane for it first. No need to add the test.

phoracek · 2020-03-13T21:31:42Z

data/macvtap/002-macvtap-daemonset.yaml

+          requests:
+            cpu: "60m"
+            memory: "30Mi"
+          limits:


Please drop limits. It causes obscure bugs on OpenShift. requests is enough

phoracek · 2020-03-13T21:52:39Z

data/macvtap/002-macvtap-daemonset.yaml

@@ -0,0 +1,63 @@
+---
+kind: ConfigMap


Disadvantage is that we spoil the generic code with "if macvtap DP".

I was writing a guide on "how to hack it", but I think there is an elegant way too.

So this issue where we want to keep user changes on objects deployed is pretty common, although we were successful avoiding addressing it, until now.

So let's think how can we make it generic in the CNAO framework and keep everything specific to the component under data/ or pkg/network.

Option A: The simplest fit into the current API would be to introduce a Kubernetes object label networkaddonsoperator.network.kubevirt.io/untouchable: "true" (or any better name after /). That can be set by a user who wants to debug manual changes on any deployed component (e.g. they want to change resource requests, add debugging flags to running apps etc). It can be also set by default under data/ manifests. The only code we need to introduce then would live around https://github.com/kubevirt/cluster-network-addons-operator/blob/master/pkg/apply/apply.go#L37, where we check if an existing object we want to update has this flag, if it does, we ignore it. If you implement it, please do it in a separate PR (in case we need to backport it) and document it at the bottom of the README.

I am tempted to say "let's just keep it as it is and wait for the proper API", but that would make it harder to test under kubevirt and if somebody unaware uses it (despite it us "experimental", they may get hurt).

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

Also deploy the default configuration of the macvtap feature. Signed-off-by: Miguel Duarte Barroso <[email protected]>

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

As a result, when you apply the generated CNAO manifest, it will also deploy the macvtap-cni daemonsets and configuration. Signed-off-by: Miguel Duarte Barroso <[email protected]>

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

maiqueb

Option A: The simplest fit into the current API would be to introduce a Kubernetes object label networkaddonsoperator.network.kubevirt.io/untouchable: "true" (or any better name after /). That can be set by a user who wants to debug manual changes on any deployed component (e.g. they want to change resource requests, add debugging flags to running apps etc). It can be also set by default under data/ manifests. The only code we need to introduce then would live around https://github.com/kubevirt/cluster-network-addons-operator/blob/master/pkg/apply/apply.go#L37, where we check if an existing object we want to update has this flag, if it does, we ignore it. If you implement it, please do it in a separate PR (in case we need to backport it) and document it at the bottom of the README.

I am tempted to say "let's just keep it as it is and wait for the proper API", but that would make it harder to test under kubevirt and if somebody unaware uses it (despite it us "experimental", they may get hurt).

Whatever the choice, it'll happen on a follow up set of patches.

I'm quite OK with adding that label.

maiqueb · 2020-03-15T18:54:53Z

data/macvtap/002-macvtap-daemonset.yaml

+          requests:
+            cpu: "60m"
+            memory: "30Mi"
+          limits:


phoracek

/lgtm
/approve

kubevirt-bot · 2020-03-17T10:51:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek

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

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

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 12, 2020

kubevirt-bot requested review from qinqon and RamLavi March 12, 2020 16:21

kubevirt-bot added size/L release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 12, 2020

kubevirt-bot requested a review from phoracek March 12, 2020 16:50

maiqueb force-pushed the integrate_macvtap_cni branch 2 times, most recently from 78637c8 to c7e51bd Compare March 13, 2020 13:47

qinqon requested changes Mar 13, 2020

View reviewed changes

kubevirt-bot assigned qinqon Mar 13, 2020

maiqueb force-pushed the integrate_macvtap_cni branch from c7e51bd to dd3517d Compare March 13, 2020 14:46

maiqueb requested a review from qinqon March 13, 2020 14:47

maiqueb force-pushed the integrate_macvtap_cni branch from dd3517d to 6fc524a Compare March 13, 2020 14:57

phoracek requested changes Mar 13, 2020

View reviewed changes

kubevirt-bot assigned phoracek Mar 13, 2020

maiqueb force-pushed the integrate_macvtap_cni branch from 6fc524a to b2d5234 Compare March 13, 2020 16:45

maiqueb commented Mar 13, 2020

View reviewed changes

maiqueb force-pushed the integrate_macvtap_cni branch from b2d5234 to e9b03d3 Compare March 13, 2020 16:49

maiqueb requested a review from phoracek March 13, 2020 16:50

maiqueb mentioned this pull request Mar 13, 2020

Make components macvtap #348

Merged

maiqueb commented Mar 13, 2020

View reviewed changes

phoracek reviewed Mar 13, 2020

View reviewed changes

maiqueb force-pushed the integrate_macvtap_cni branch from 0e76c7a to e659087 Compare March 16, 2020 10:59

maiqueb added 3 commits March 16, 2020 12:05

macvtap-cni: add manifests

83aa875

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

macvtap-cni: enable deployment of the macvtap DS

d599073

Also deploy the default configuration of the macvtap feature. Signed-off-by: Miguel Duarte Barroso <[email protected]>

macvtap-cni: add macvtap-cni to manifest templator

35b3fb7

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

maiqueb added 4 commits March 16, 2020 12:05

macvtap-cni: add macvtap-cni to CNAO default configuration

1b69fbe

As a result, when you apply the generated CNAO manifest, it will also deploy the macvtap-cni daemonsets and configuration. Signed-off-by: Miguel Duarte Barroso <[email protected]>

macvtap-cni: test integration w/ CNAO

63a4d89

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

go fmt on network pkg

d6c7247

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

macvtap-cni: update the README

1322900

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

maiqueb force-pushed the integrate_macvtap_cni branch from e659087 to 1322900 Compare March 16, 2020 11:05

maiqueb commented Mar 16, 2020

View reviewed changes

data/macvtap/002-macvtap-daemonset.yaml Outdated

requests:

cpu: "60m"

memory: "30Mi"

limits:

Copy link

Contributor Author

maiqueb Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

maiqueb requested a review from phoracek March 16, 2020 14:16

phoracek approved these changes Mar 17, 2020

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2020

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2020

kubevirt-bot merged commit 2232143 into kubevirt:master Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate macvtap cni #346

Integrate macvtap cni #346

maiqueb commented Mar 12, 2020 •

edited

Loading

maiqueb commented Mar 12, 2020

qinqon Mar 13, 2020

maiqueb Mar 13, 2020 •

edited

Loading

phoracek left a comment

phoracek Mar 13, 2020

maiqueb Mar 13, 2020

phoracek Mar 13, 2020

maiqueb left a comment

maiqueb Mar 13, 2020

maiqueb Mar 13, 2020 •

edited

Loading

maiqueb commented Mar 13, 2020

maiqueb Mar 13, 2020 •

edited

Loading

phoracek Mar 13, 2020

phoracek Mar 13, 2020

phoracek Mar 13, 2020

maiqueb Mar 15, 2020

phoracek Mar 13, 2020

maiqueb left a comment

maiqueb Mar 15, 2020

phoracek left a comment

kubevirt-bot commented Mar 17, 2020

Integrate macvtap cni #346

Integrate macvtap cni #346

Conversation

maiqueb commented Mar 12, 2020 • edited Loading

maiqueb commented Mar 12, 2020

Choose a reason for hiding this comment

maiqueb Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

phoracek 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

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

maiqueb commented Mar 13, 2020

maiqueb Mar 13, 2020 • edited Loading

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

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoracek left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Mar 17, 2020

maiqueb commented Mar 12, 2020 •

edited

Loading

maiqueb Mar 13, 2020 •

edited

Loading

maiqueb Mar 13, 2020 •

edited

Loading

maiqueb Mar 13, 2020 •

edited

Loading