Add HostDeviceNetwork basic support #126

e0ne · 2021-03-01T20:28:46Z

This patch adds HostDeviceNetwork CRD support and basic
transformation to NetworkAttachmentDefinition to allow
Multus create secondary network.

SR-IOV Network Device Plugin is deployed using Network Operator
Helm chart with default configuration.

NOTE: temporary added "//nolint:dupl" before code will be
refactored.

Closes: #44

api/v1alpha1/hostdevicenetwork_types.go

example/crs/mellanox.com_v1alpha1_hostdevicenetwork_cr.yaml

pkg/state/factory.go

pkg/state/state_hostdevice_network.go

moshe010 · 2021-03-03T21:56:39Z

can you add in the commit msg that you are fix #44

e0ne · 2021-03-09T19:34:53Z

Removed WIP to get more reviews. It's still required to remove duplicated code and add unit-tests

api/v1alpha1/nicclusterpolicy_types.go

config/crd/bases/mellanox.com_hostdevicenetworks.yaml

api/v1alpha1/hostdevicenetwork_types.go

manifests/stage-hostdevice-network/0010-hostdevice-net-cr.yml

manifests/stage-sriov-device-plugin/0020-sriov-dp-daemonset.yml

adrianchiris · 2021-03-10T15:24:03Z

manifests/stage-sriov-device-plugin/0020-sriov-dp-daemonset.yml

+      nodeSelector:
+        beta.kubernetes.io/arch: amd64
+      tolerations:
+        - key: node-role.kubernetes.io/master


in rdma shared DP we got the following tolerations:

tolerations: # Allow this pod to be rescheduled while the node is in "critical add-ons only" mode. # This, along with the annotation above marks this pod as a critical add-on. - key: CriticalAddonsOnly operator: Exists - key: node-role.kubernetes.io/master operator: Exists effect: NoSchedule - key: nvidia.com/gpu operator: Exists effect: NoSchedule

these should be aligned.

also i think we want:

nodeSelector: feature.node.kubernetes.io/pci-15b3.present: "true"

as well.

adrianchiris · 2021-03-10T15:30:06Z

pkg/state/state_hostdevice_network.go

+)
+
+const (
+	stateHostDeviceNetworkName        = "state-HostDevice-Network"


i realize we are not consistent here throughout the states perhaps we should settle on state-word-another-word

later on aligning the rest WDYT?

+1. Let's keep consistency in the code

adrianchiris · 2021-03-10T15:36:17Z

pkg/state/state_shared_dp.go

 		// Either this state was not required to run or an update occurred and we need to remove
 		// the resources that where created.
-		// TODO: Support the latter case
+		// TODO: Support default configuration should be applied


what are the default configurations ?

the original intention was that if this is nil then this state should be "undone" i.e removal of all yaml in this state's manifest folder

moshe010

I don't review all. I will continue later

moshe010 · 2021-03-11T23:23:14Z

api/v1alpha1/hostdevicenetwork_types.go

+}
+
+// +kubebuilder:object:root=true
+// kubebuilder:object:generate


what is kubebuilder:object:generate
why you don't +kubebuilder:object:generate= it ?

It works using both patterns but I agree that it's good to use only on across the project

api/v1alpha1/hostdevicenetwork_types.go

moshe010 · 2021-03-11T23:46:00Z

api/v1alpha1/hostdevicenetwork_types.go

+}
+
+// +kubebuilder:object:root=true
+// kubebuilder:object:generate


same as above

config/crd/bases/mellanox.com_hostdevicenetworks.yaml

controllers/hostdevicenetwork_controller.go

moshe010 · 2021-03-12T10:07:53Z

api/v1alpha1/hostdevicenetwork_types.go

+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	Spec   *HostDeviceNetworkSpec  `json:"spec,omitempty"`


why the Spec is with *?
I don't see it like this in https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/api/v1/sriovibnetwork_types.go#L49

moshe010 · 2021-03-12T10:14:38Z

deployment/network-operator/crds/mellanox.com_hostdevicenetworks.yaml

+      name: v1alpha1
+      schema:
+        openAPIV3Schema:
+          description: MacvlanNetwork is the Schema for the macvlannetworks API


s/HostDevice/MacvlanNetwork
s/macvlannetwork/hostdevicenetworks

deployment/network-operator/crds/mellanox.com_hostdevicenetworks.yaml

e0ne · 2021-03-12T14:43:11Z

Note for reviewers: Most of the comments are addressed and squashed into one commit. HostDeviceNetwork scope and unit-tests will be added as a separate commits in this PR

api/v1alpha1/hostdevicenetwork_types.go

config/rbac/role.yaml

deployment/network-operator/templates/mellanox.com_v1alpha1_nicclusterpolicy_cr.yaml

moshe010 · 2021-03-16T20:26:16Z

pkg/state/factory.go

+		return nil, errors.Wrapf(err, "failed to create Shared Device plugin State")
+	}
+	sriovDpState, err := NewStateSriovDp(
+		k8sAPIClient, scheme, filepath.Join(manifestBaseDir, "stage-sriov-device-plugin"))
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to create Device plugin State")


s/Device plugin/SR-IOV Device plugin/

e0ne · 2021-03-17T16:24:34Z

/retest-nic_operator

moshe010 · 2021-03-17T18:09:07Z

api/v1alpha1/hostdevicenetwork_types.go

+type HostDeviceNetworkSpec struct {
+	// Namespace of the NetworkAttachmentDefinition custom resource
+	NetworkNamespace string `json:"networkNamespace,omitempty"`
+	// Endpoint resource name for SR-IOV resource pool


maybe reword to host device resource pool

I borrowed it from the SR-IOV network device plugin documentation, but this name it good too

moshe010 · 2021-03-17T18:12:00Z

config/crd/bases/mellanox.com_hostdevicenetworks.yaml

+                description: Namespace of the NetworkAttachmentDefinition custom resource
+                type: string
+              resourceName:
+                description: Endpoint resource name for SR-IOV resource pool


/SR-IOV/host device

config/crd/bases/mellanox.com_nicclusterpolicies.yaml

moshe010 · 2021-03-17T19:09:32Z

Also please add Closes #44 to commit msg

e0ne · 2021-03-17T21:53:01Z

Also please add Closes #44 to commit msg

It's already added. I'll add it to the PR description too

pkg/state/state_shared_dp.go

moshe010

@adrianchiris can you please take a look

moshe010 · 2021-03-26T22:00:22Z

pkg/state/state_hostdevice_network_test.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2020 NVIDIA


s/2020/2021

moshe010 · 2021-03-26T22:01:39Z

pkg/state/state_sriov_dp_test.go

@@ -0,0 +1,110 @@
+/*
+Copyright 2020 NVIDIA


s/2020/2021

pkg/state/state_sriov_dp_test.go

moshe010 · 2021-03-26T22:05:57Z

scripts/update-gh-pages.sh

@@ -0,0 +1,209 @@
+#!/bin/bash -e


this doesn't seem to be releated to PR also I see network-operator.0.3.1.tgz binary which should be removed

Yes. It was accidently added to the PR

This patch adds HostDeviceNetwork CRD support and basic transformation to NetworkAttachmentDefinition to allow Multus create secondary network. SR-IOV Network Device Plugin is deployed using Network Operator Helm chart with default configuration. This patch also optimize NetworkAttachmentDef link generation both for MacvlanNetwork and HostDeviceNetwork CRs. We should add link for NetworkAttachmentDef only if MacvlanNetwork or HostDeviceNetwork CR is in a 'Ready' state. It allows us to make less Kubernetes API calls. NOTE: temporary added "//nolint:dupl" before code will be refactored. Closes: Mellanox#44 Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>

e0ne force-pushed the host-net branch 3 times, most recently from 03bbd3b to ff532e6 Compare March 2, 2021 15:21

moshe010 requested changes Mar 2, 2021

View reviewed changes

e0ne force-pushed the host-net branch 5 times, most recently from b855226 to 4ddc2d2 Compare March 2, 2021 22:39

e0ne force-pushed the host-net branch 5 times, most recently from 7f14ced to 255a38a Compare March 9, 2021 19:33

e0ne changed the title ~~[WIP] Add HostDeviceNetwork basic support~~ Add HostDeviceNetwork basic support Mar 9, 2021

e0ne force-pushed the host-net branch from 255a38a to e0ad19b Compare March 9, 2021 20:47

adrianchiris requested changes Mar 10, 2021

View reviewed changes

moshe010 requested changes Mar 12, 2021

View reviewed changes

moshe010 mentioned this pull request Mar 12, 2021

Release v0.4.0 #137

Closed

34 tasks

moshe010 requested changes Mar 12, 2021

View reviewed changes

e0ne force-pushed the host-net branch from e0ad19b to d81d36a Compare March 12, 2021 14:41

e0ne force-pushed the host-net branch 6 times, most recently from ae437e0 to 09f598a Compare March 15, 2021 10:13

e0ne force-pushed the host-net branch from 09f598a to 2bd625d Compare March 15, 2021 20:11

moshe010 requested changes Mar 16, 2021

View reviewed changes

e0ne force-pushed the host-net branch from 2bd625d to e54f427 Compare March 17, 2021 11:39

moshe010 reviewed Mar 17, 2021

View reviewed changes

e0ne force-pushed the host-net branch 4 times, most recently from f2a4e1a to a2322a2 Compare March 18, 2021 11:02

moshe010 reviewed Mar 18, 2021

View reviewed changes

pkg/state/state_shared_dp.go Show resolved Hide resolved

e0ne force-pushed the host-net branch from a2322a2 to b990c6f Compare March 18, 2021 12:56

moshe010 approved these changes Mar 18, 2021

View reviewed changes

e0ne force-pushed the host-net branch 5 times, most recently from 884366c to 5c73a4e Compare March 25, 2021 09:39

moshe010 requested changes Mar 26, 2021

View reviewed changes

e0ne force-pushed the host-net branch from 19384cc to 67ac036 Compare March 27, 2021 16:03

moshe010 approved these changes Mar 29, 2021

View reviewed changes

moshe010 merged commit 6788484 into Mellanox:master Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HostDeviceNetwork basic support #126

Add HostDeviceNetwork basic support #126

e0ne commented Mar 1, 2021 •

edited

Loading

moshe010 commented Mar 3, 2021

e0ne commented Mar 9, 2021

adrianchiris Mar 10, 2021

adrianchiris Mar 10, 2021

adrianchiris Mar 10, 2021

e0ne Mar 12, 2021

adrianchiris Mar 10, 2021

moshe010 left a comment

moshe010 Mar 11, 2021

e0ne Mar 12, 2021

moshe010 Mar 11, 2021

moshe010 Mar 12, 2021

e0ne Mar 12, 2021 •

edited

Loading

moshe010 Mar 12, 2021

e0ne commented Mar 12, 2021

moshe010 Mar 16, 2021

e0ne Mar 17, 2021

e0ne commented Mar 17, 2021

moshe010 Mar 17, 2021

e0ne Mar 17, 2021

moshe010 Mar 17, 2021

moshe010 commented Mar 17, 2021

e0ne commented Mar 17, 2021

moshe010 left a comment

moshe010 Mar 26, 2021

moshe010 Mar 26, 2021

moshe010 Mar 26, 2021

e0ne Mar 27, 2021

Add HostDeviceNetwork basic support #126

Add HostDeviceNetwork basic support #126

Conversation

e0ne commented Mar 1, 2021 • edited Loading

moshe010 commented Mar 3, 2021

e0ne commented Mar 9, 2021

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

moshe010 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

e0ne Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e0ne commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e0ne commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe010 commented Mar 17, 2021

e0ne commented Mar 17, 2021

moshe010 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

e0ne commented Mar 1, 2021 •

edited

Loading

e0ne Mar 12, 2021 •

edited

Loading