multus: add support for the Multus daemonset #54

dcbw · 2019-01-08T18:32:03Z

Multus will be installed by default but can be turned off by
setting the DisableMultiNetwork property of the Cluster Network Operator
object to 'true'.

[NOTE: we are waiting on https://github.com/openshift/release/pull/2570 to land, which will provide the official images for Multus. Once that lands, I will remove the "hack: no images for now" commit and update the image references.]

bindata/network/multus/multus.yaml

squeed · 2019-01-08T21:39:25Z

bindata/network/multus/multus.yaml

+      serviceAccountName: multus
+      containers:
+      - name: kube-multus
+        image: nfvpe/multus:latest


You probably know this, but the image url will need to be templated through like the others.

Do we want to use nfvpe/multus or pull from official registry, one of the issue I hit was that occasionally the latest nfvpe/multus image doesn't support reading kubelet device plugin checkpoint file which results in failure of configuring sriov-cni. not sure if it's because nfvpe/multus:latest may be updated by builds from different branches.

yep, hence the WIP :)

squeed · 2019-01-08T21:42:57Z

pkg/network/render.go

@@ -185,9 +213,9 @@ func RenderAdditionalNetworks(conf *netv1.NetworkConfigSpec, manifestDir string)
 		return nil, errors.Errorf("invalid Additional Network Configuration: %v", errs)
 	}

-	// render Multus when additional networks is provided
+	// render the CRD when additional networks are provided


As per our previous discussion, we need to render the CRD whenever Multus is enabled so administrators can create additional networks out-of-band. I suspect that Multus also needs the CRD to exist to prevent errors.

I don't think Multus cares. It only looks for the pod annotations, and only then does it try to get the networks using the CRD. So it's not going to block pods that don't use multiple networks. But yeah, CRD should get rendered anyway.

bindata/network/multus/multus.yaml

zshi-redhat · 2019-01-09T02:52:11Z

bindata/network/multus/multus.yaml

+      serviceAccountName: multus
+      containers:
+      - name: kube-multus
+        image: nfvpe/multus:latest


Do we want to use nfvpe/multus or pull from official registry, one of the issue I hit was that occasionally the latest nfvpe/multus image doesn't support reading kubelet device plugin checkpoint file which results in failure of configuring sriov-cni. not sure if it's because nfvpe/multus:latest may be updated by builds from different branches.

bindata/network/multus/multus.yaml

zshi-redhat · 2019-01-09T03:01:45Z

pkg/network/render.go

 	if len(ans) > 0 {
-		objs, err := renderMultusConfig(manifestDir)
+		objs, err := renderAdditionalNetworksCRD(manifestDir)


Rendering of CRD and CR may fail in a single run, some details here

@zshi-redhat yeah, that's pretty odd and I think a bug somewhere in the Kube parts. If we create the CRD first, there's no reason that creating a subsequent CR should fail.

@dcbw I still see this failure with the latest version of PR, not sure if it's specified to how I run the test (run the binary of cluster-network-operator in one of the master nodes). did you ever hit this issue?

zshi-redhat · 2019-01-09T03:05:49Z

pkg/network/render.go

@@ -41,6 +48,7 @@ func Validate(conf *netv1.NetworkConfigSpec) error {
 	errs := []error{}

 	errs = append(errs, ValidateDefaultNetwork(conf)...)
+	errs = append(errs, ValidateMultus(conf)...)


Shall we also validate additional networks here? If it's not provided, validation of additional network returns immediately.

@zshi-redhat no, the validate of additional networks would be handled by the additional network rendering, not multus rendering. Multus and AdditionalNetworks are completely separate codepaths that do not depend on each other.

Sorry, I added the comment in the wrong line, I mean add validation of additional network in a separate line so that controller will capture the failure immediately instead of reaching the render phase. just little optimization as it will anyway be validated before rendering.

pkg/network/render.go

dcbw · 2019-01-09T03:27:10Z

We probably want to make Multus conf file more configurable instead of using auto, reason being that there are some parameters that user may want to configure, for example, the namespaceIsolation, logLevel, logFile etc and in the future when enabling network migration with Multus, user may want to specify default network plugin although we may support configure default network with net-attach-def CR.

At this point, let's wait until we actually have use-cases for those things before we put them into the operator. I honestly don't think namespaceIsolation is worthwhile here, that's what the Admission Controller and RBAC is for. logLevel and logFile would be controlled by the Network operator when increased debugging is called for, but shouldn't be a user-visible option. Almost everything that multus allows configuration for, should be handled automatically by the operator and not be a passthrough option to the user.

zshi-redhat · 2019-01-09T03:32:24Z

We probably want to make Multus conf file more configurable instead of using auto, reason being that there are some parameters that user may want to configure, for example, the namespaceIsolation, logLevel, logFile etc and in the future when enabling network migration with Multus, user may want to specify default network plugin although we may support configure default network with net-attach-def CR.

At this point, let's wait until we actually have use-cases for those things before we put them into the operator. I honestly don't think namespaceIsolation is worthwhile here, that's what the Admission Controller and RBAC is for. logLevel and logFile would be controlled by the Network operator when increased debugging is called for, but shouldn't be a user-visible option. Almost everything that multus allows configuration for, should be handled automatically by the operator and not be a passthrough option to the user.

Ok, understood, Operator would be the only user-facing interface, if there needs to be an user-configurable parameter to multus, it will be exposed in the operator level.

dcbw · 2019-01-09T03:42:48Z

Ok, understood, Operator would be the only user-facing interface, if there needs to be an user-configurable parameter to multus, it will be exposed in the operator level.

@zshi-redhat yeah, at least for now. Let's keep things simple because once we add something, we can't easily remove it :)

dcbw · 2019-01-10T14:49:58Z

/retest

flake is during install:

time="2019-01-10T07:40:24Z" level=error
time="2019-01-10T07:40:24Z" level=error msg="Error: Error applying plan:"
time="2019-01-10T07:40:24Z" level=error
time="2019-01-10T07:40:24Z" level=error msg="1 error occurred:"
time="2019-01-10T07:40:24Z" level=error msg="\t* module.vpc.aws_route.to_nat_gw[4]: 1 error occurred:"
time="2019-01-10T07:40:24Z" level=error msg="\t* aws_route.to_nat_gw.4: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"

dcbw · 2019-01-10T21:41:53Z

/retest

dcbw · 2019-01-10T21:52:21Z

/retest

dcbw · 2019-01-10T21:59:49Z

/retest

squeed · 2019-01-16T13:09:50Z

/lgtm

dcbw · 2019-01-17T21:34:02Z

NOTE: we are waiting on openshift/release#2570 to land, which will provide the official images for Multus. Once that lands, I will remove the "hack: no images for now" commit and update the image references. But this PR demonstrates that Multus passes CI even without official images.

dcbw · 2019-01-24T07:34:07Z

@danwinship real images added now, FWIW

knobunc · 2019-01-24T14:15:26Z

/retest

manifests/image-references

bindata/network/multus/000-ns.yaml

bindata/network/multus/multus.yaml

Multus will be installed by default but can be turned off by setting the DisableMultiNetwork property of the Cluster Network Operator object to 'true'.

dcbw · 2019-01-25T11:46:39Z

/retest
One master failed to correctly download images for many containers. ImageInspectError too, no idea what that means in practice.

kube-system                          etcd-member-ip-10-0-26-81.ec2.internal                    1/1       Running             0          17m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>
multus                               multus-w72wg                                              0/1       CrashLoopBackOff    9          22m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>
openshift-cluster-network-operator   cluster-network-operator-htwh8                            1/1       Running             0          22m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>
openshift-dns                        dns-default-968p5                                         0/2       ContainerCreating   0          21m       <none>       ip-10-0-26-81.ec2.internal   <none>
openshift-kube-apiserver             installer-1-ip-10-0-26-81.ec2.internal                    0/1       ContainerCreating   0          17m       <none>       ip-10-0-26-81.ec2.internal   <none>
openshift-sdn                        ovs-cpvss                                                 0/1       ImageInspectError   0          22m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>
openshift-sdn                        sdn-controller-4tx5t                                      1/1       Running             0          22m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>
openshift-sdn                        sdn-dz8n4                                                 0/1       ImageInspectError   0          22m       10.0.26.81   ip-10-0-26-81.ec2.internal   <none>

dcbw · 2019-01-25T12:53:28Z

/retest

dcbw · 2019-01-25T12:54:38Z

@danwinship look better now?

danwinship · 2019-01-25T15:23:21Z

/lgtm

openshift-ci-robot · 2019-01-25T15:23:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, squeed

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~~ [danwinship,dcbw,squeed]

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

knobunc · 2019-01-25T21:05:39Z

/test all

knobunc · 2019-01-25T21:32:50Z

/hold cancel

openshift-bot · 2019-01-26T09:48:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

dcbw · 2019-01-26T10:24:03Z

@dcbw: The following test failed, say /retest to rerun them all:
Test name Commit Details Rerun command
ci/prow/e2e-aws fd9303c link /test e2e-aws

The following don't seem network related...

Failing tests:

[sig-apps] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] should not deadlock when a pod's predecessor fails [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Volume limits should verify that all nodes have volume limits [Suite:openshift/conformance/parallel] [Suite:k8s]

openshift-bot · 2019-01-26T22:04:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jeremyeder · 2019-01-27T01:31:59Z

+100

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2019

openshift-ci-robot requested review from JacobTanenbaum and pecameron January 8, 2019 18:33

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2019

dcbw mentioned this pull request Jan 8, 2019

multus: add multus manifests #53

Closed

dougbtv reviewed Jan 8, 2019

View reviewed changes

bindata/network/multus/multus.yaml Outdated Show resolved Hide resolved

squeed reviewed Jan 8, 2019

View reviewed changes

zshi-redhat reviewed Jan 9, 2019

View reviewed changes

dcbw force-pushed the multus branch 2 times, most recently from 4f01da2 to 555d35f Compare January 10, 2019 06:17

dcbw changed the title ~~[WIP] multus: add support for the Multus daemonset~~ multus: add support for the Multus daemonset Jan 10, 2019

dcbw force-pushed the multus branch from 69421fc to 4211d3a Compare January 10, 2019 07:25

dcbw force-pushed the multus branch 2 times, most recently from e634bea to 555d35f Compare January 10, 2019 19:59

openshift-ci-robot assigned squeed Jan 16, 2019

danwinship approved these changes Jan 17, 2019

View reviewed changes

dcbw force-pushed the multus branch from de019f5 to b6047c4 Compare January 24, 2019 07:33

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2019

dcbw force-pushed the multus branch from b6047c4 to 6e9b424 Compare January 24, 2019 08:21

knobunc reviewed Jan 24, 2019

View reviewed changes

manifests/image-references Outdated Show resolved Hide resolved

dcbw force-pushed the multus branch from 6e9b424 to ee5ef60 Compare January 24, 2019 16:21

squeed approved these changes Jan 24, 2019

View reviewed changes

danwinship requested changes Jan 24, 2019

View reviewed changes

bindata/network/multus/000-ns.yaml Show resolved Hide resolved

bindata/network/multus/multus.yaml Outdated Show resolved Hide resolved

bindata/network/multus/multus.yaml Outdated Show resolved Hide resolved

dcbw added 2 commits January 25, 2019 11:40

multus: add support for the Multus daemonset

869a68d

Multus will be installed by default but can be turned off by setting the DisableMultiNetwork property of the Cluster Network Operator object to 'true'.

cni: add supported & unsupported plugins

fd9303c

dcbw force-pushed the multus branch from ee5ef60 to fd9303c Compare January 25, 2019 10:40

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2019

openshift-merge-robot merged commit cfcc522 into openshift:master Jan 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multus: add support for the Multus daemonset #54

multus: add support for the Multus daemonset #54

dcbw commented Jan 8, 2019 •

edited

squeed Jan 8, 2019

zshi-redhat Jan 9, 2019

dcbw Jan 9, 2019

squeed Jan 8, 2019

dcbw Jan 9, 2019 •

edited

zshi-redhat Jan 9, 2019

zshi-redhat Jan 9, 2019

dcbw Jan 9, 2019

zshi-redhat Jan 15, 2019

zshi-redhat Jan 9, 2019

dcbw Jan 9, 2019

zshi-redhat Jan 9, 2019

dcbw commented Jan 9, 2019

zshi-redhat commented Jan 9, 2019

dcbw commented Jan 9, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

squeed commented Jan 16, 2019

dcbw commented Jan 17, 2019

dcbw commented Jan 24, 2019

knobunc commented Jan 24, 2019

dcbw commented Jan 25, 2019 •

edited

dcbw commented Jan 25, 2019

dcbw commented Jan 25, 2019

danwinship commented Jan 25, 2019

openshift-ci-robot commented Jan 25, 2019

knobunc commented Jan 25, 2019

knobunc commented Jan 25, 2019

openshift-bot commented Jan 26, 2019

dcbw commented Jan 26, 2019 •

edited

openshift-bot commented Jan 26, 2019

jeremyeder commented Jan 27, 2019

multus: add support for the Multus daemonset #54

multus: add support for the Multus daemonset #54

Conversation

dcbw commented Jan 8, 2019 • edited

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

dcbw Jan 9, 2019 • edited

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

dcbw commented Jan 9, 2019

zshi-redhat commented Jan 9, 2019

dcbw commented Jan 9, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

dcbw commented Jan 10, 2019

squeed commented Jan 16, 2019

dcbw commented Jan 17, 2019

dcbw commented Jan 24, 2019

knobunc commented Jan 24, 2019

dcbw commented Jan 25, 2019 • edited

dcbw commented Jan 25, 2019

dcbw commented Jan 25, 2019

danwinship commented Jan 25, 2019

openshift-ci-robot commented Jan 25, 2019

knobunc commented Jan 25, 2019

knobunc commented Jan 25, 2019

openshift-bot commented Jan 26, 2019

dcbw commented Jan 26, 2019 • edited

openshift-bot commented Jan 26, 2019

jeremyeder commented Jan 27, 2019

dcbw commented Jan 8, 2019 •

edited

dcbw Jan 9, 2019 •

edited

dcbw commented Jan 25, 2019 •

edited

dcbw commented Jan 26, 2019 •

edited