OVS HWOL: use generic plugin to write switchdev device configuration #106

zshi-redhat · 2021-04-11T06:37:09Z

Added SriovOperatorConfig.Spec.OvsHardwareOffload API field to define Labels that matches with MachineConfigPool NodeSelector. Nodes that's selected by the Labels will be applied with "bindata/manifest/switchdev-config".

Removed MCO plugins and moved the writing of switchdev device config file into generic plugin (shared by OpenShift and Kubernetes deployment). Removed the writing of switchdev device config file from K8s plugin.

For configuring OVS HWOL on OpenShift, the steps are changed to:

Configured default SriovOperatorConfig to apply switchdev-config services to MachineConfigPool
Configure SriovNetworkNodePolicy with eSwitchMode: switchdev (This writes switchdev.conf file and trigger drain/reboot where needed)

For configuring OVS HWOL on Kubernetes, the steps are not changed.

lgtm-com · 2021-04-11T06:52:39Z

This pull request introduces 1 alert when merging 2356622 into f17625b - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

zshi-redhat · 2021-04-11T08:53:59Z

/cc @Mmduh-483 @pliurh

Mmduh-483

Did you test the code on k8s cluster?

Mmduh-483 · 2021-04-12T05:59:19Z

pkg/plugins/generic/generic_plugin.go

@@ -212,3 +196,25 @@ func needDrainNode(desired sriovnetworkv1.Interfaces, current sriovnetworkv1.Int
 	}
 	return
 }
+
+func needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState, loadVfioDriver *uint) (needReboot bool) {


Can you add logs in this function why reboot is needed

Mmduh-483 · 2021-04-12T06:03:20Z

pkg/plugins/k8s/k8s_plugin.go

@@ -127,24 +127,6 @@ func (p *K8sPlugin) OnNodeStateChange(old, new *sriovnetworkv1.SriovNetworkNodeS
 		}
 	}

-	// Check switchdev config


K8s plugin is only checking the services now, can you reformat the condition on line 121 to be
if !utils.IsSwitchdevModeSpec(new.Spec) {
continue
}

Can you also remove the word "not" in the comment in line 120

@Mmduh-483 I thought we still need the p.servicesStateUpdate() to set the right flags or content for K8s_plugin struct, isn't it? Otherwise, in line p.updateTarget.needUpdate() , it will always be false.

In the current implementation:

p.servicesStateUpdate runs when if condition utils.IsSwitchdevModeSpec(new.Spec) is met, I suggest a simple refactoring for the if condtion

At the start of OnNOdeChange p.updateTarget is reset to be all false p.updateTarget.needUpdate() is always false if there is no switchdev spec

So your suggestion is to change:

if utils.IsSwitchdevModeSpec(new.Spec) { // Check services err = p.servicesStateUpdate() if err != nil { glog.Errorf("k8s-plugin OnNodeStateChange(): failed : %v", err) return } }

To:

if !utils.IsSwitchdevModeSpec(new.Spec) { continue } err = p.servicesStateUpdate() if err != nil { glog.Errorf("k8s-plugin OnNodeStateChange(): failed : %v", err) return }

Is it correct?

Mmduh-483 · 2021-04-12T06:05:49Z

pkg/daemon/daemon.go

 		glog.V(0).Infof("nodeStateSyncHandler(): plugin %s: reqDrain %v, reqReboot %v", k, d, r)
 		reqDrain = reqDrain || d
 		reqReboot = reqReboot || r
 	}
 	glog.V(0).Infof("nodeStateSyncHandler(): reqDrain %v, reqReboot %v disableDrain %v", reqDrain, reqReboot, dn.disableDrain)

 	for k, p := range dn.LoadedPlugins {
-		if k != GenericPlugin && k != McoPlugin {
+		if k != GenericPlugin {
+			if k == K8sPlugin && utils.ClusterType == utils.ClusterTypeOpenshift {


Is there a reason why this condition needed for the k8s plugin and openshift cluster type?
on line 663 you ensure that plugin is not loaded

Will remove, there is no need for this conditional check.

zshi-redhat · 2021-04-12T09:38:02Z

Did you test the code on k8s cluster?

I didn't, I only tried on OpenShift.

Mmduh-483 · 2021-04-13T06:49:18Z

@zshi-redhat lgtm for me, can you verify it is working on k8s cluster?

zshi-redhat · 2021-04-15T00:43:27Z

@zshi-redhat lgtm for me, can you verify it is working on k8s cluster?

@Mmduh-483 I'd like to, but I don't have an env that can be easily deployed with k8s.

pliurh · 2021-04-15T07:18:04Z

controllers/sriovoperatorconfig_controller.go

+		// MachineConfigPool is selected when its NodeSelectors are included in MachineConfigPoolSelector
+		for _, pool := range dc.Spec.OvsHardwareOffload {
+			if selector.Matches(labels.Set(pool.MachineConfigPoolSelector)) {
+				mcpMap[mcp.GetName()] = true


Shall we store this information in the status, so that users can know which MCP is matched?

pliurh · 2021-04-15T07:26:05Z

api/v1/sriovoperatorconfig_types.go

@@ -24,6 +24,15 @@ type SriovOperatorConfigSpec struct {
 	DisableDrain bool `json:"disableDrain,omitempty"`
 	// Flag to enable OVS hardware offload. Set to 'true' to provision switchdev-configuration.service and enable OpenvSwitch hw-offload on nodes.
 	EnableOvsOffload bool `json:"enableOvsOffload,omitempty"`
+	// OvsHardwareOffload describes the OVS HWOL configuration for selected MachineConfigPool
+	OvsHardwareOffload []OvsHardwareOffloadConfig `json:"ovsHardwareOffload,omitempty"`


I haven't seen such an API design in K8S API. Normally, if users need to select objects, they shall use a selector which matches the label of the objects. Personally, I think this implementation is hard to understand.

Shall we ask users to label the MCP by themself, and we use a selector here?

I haven't seen such an API design in K8S API. Normally, if users need to select objects, they shall use a selector which matches the label of the objects. Personally, I think this implementation is hard to understand.

My previous thinking was that we may want to re-use the same API for k8s case in the future.
Current OvsHardwareOffload.MachineConfigPoolSelector matches on node labels, where for OpenShift, it matches the (node) labels defined in MCP Spec.NodeSelector, for k8s, it could match the node labels directly.

Shall we ask users to label the MCP by themself, and we use a selector here?

One idea I had before was to label or annotate MCP (with ovs-hw-offload) directly, then not change the SR-IOV API, instead, we can have SR-IOV Operator watch on MCP update for creating/deleting HWOL MCs. wdyt?

I haven't seen such an API design in K8S API. Normally, if users need to select objects, they shall use a selector which matches the label of the objects. Personally, I think this implementation is hard to understand.

There is a similar example in tuned Operator, which was suggested by MCO team: https://github.com/openshift/cluster-node-tuning-operator/blob/master/pkg/apis/tuned/v1/tuned_types.go#L79
It defines the MachineConfigLabels in Tuned API to match on MCPs.

My previous thinking was that we may want to re-use the same API for k8s case in the future.

If we plan to reuse it for k8s, the name MachineConfigPoolSelector is kind of misleading. How about just use NodeSelector instead?

One idea I had before was to label or annotate MCP (with ovs-hw-offload) directly, then not change the SR-IOV API, instead, we can have SR-IOV Operator watch on MCP update for creating/deleting HWOL MCs. wdyt?

I prefer we can have a common API for both OCP and K8S. Otherwise, we still need a way to tell when the ovs hw offload shall be applied in the k8s case. Currently, it's hard bonded with the switchdev configuration.

Changed to NodeSelector

pliurh · 2021-04-15T07:32:46Z

controllers/sriovoperatorconfig_controller.go

-		} else {
-			return fmt.Errorf("Failed to get MachineConfig: %v", err)
+	for mcpName, enable := range mcpMap {
+		mcName := "00-" + mcpName + "-" + OVS_HWOL_MACHINE_CONFIG_NAME_SUFFIX


Do we have to create one MC for each selected pool? Have you considered if spec.machineConfigSelector in MCP can be used instead?

Yes, with current API change, we probably can use single MC be matched by all MCP.
I was thinking for advanced cases that we might have HWOL configurations that differ between each MCP in the future (e.g. the tc_policy config), in which case, having one MC per MCP would be needed.

pliurh · 2021-04-15T07:41:38Z

pkg/plugins/generic/generic_plugin.go

+		}
+	}
+
+	update, remove, err := utils.WriteSwitchdevConfFile(state)


Why we need to distinguish 'remove' from 'update' here?

We don't, will update.

pliurh · 2021-04-16T03:05:59Z

controllers/sriovoperatorconfig_controller.go

-	foundMC := &mcfgv1.MachineConfig{}
-	foundMCP := &mcfgv1.MachineConfigPool{}
+	// Offload is not supported for master MachineConfigPool
+	delete(mcpMap, "master")


Instead of deletion, can we check the MCP name before adding to mcpMap, and log an error if users select the master pool?

Signed-off-by: Zenghui Shi <[email protected]>

One node can only be added in one custom MCP pool by design of MCO. Since current ovs-hw-offload MCs are applied to hardcoded MCP ovs-hw-offload-worker, other existing custome MCPs cannot be enabled with ovs hw offload configuration. This commits allows ovs-hw-offload MCs be applied to multiple MCPs as specified in default SriovOperatorConfig. Signed-off-by: Zenghui Shi <[email protected]>

Signed-off-by: Zenghui Shi <[email protected]>

zshi-redhat mentioned this pull request Apr 11, 2021

Don't run k8s-plugin when CLUSTER_TYPE is OpenShift #105

Closed

github-actions bot requested a review from pliurh April 11, 2021 08:54

zshi-redhat force-pushed the ovs-hw-offload-api branch from 2356622 to 2c662b7 Compare April 12, 2021 00:31

Mmduh-483 reviewed Apr 12, 2021

View reviewed changes

zshi-redhat force-pushed the ovs-hw-offload-api branch from 2c662b7 to b030b6b Compare April 13, 2021 00:47

pliurh reviewed Apr 15, 2021

View reviewed changes

pliurh reviewed Apr 16, 2021

View reviewed changes

zshi-redhat added 4 commits April 19, 2021 10:21

Add OVS HWOL Config SriovOperatorConfig API

08a4266

Signed-off-by: Zenghui Shi <[email protected]>

Remove mco plugin

4c26054

Signed-off-by: Zenghui Shi <[email protected]>

Configure switchdev.conf (numVfs) in generic plugin

047f6b5

Signed-off-by: Zenghui Shi <[email protected]>

zshi-redhat force-pushed the ovs-hw-offload-api branch from b030b6b to 047f6b5 Compare April 19, 2021 08:49

zshi-redhat mentioned this pull request May 7, 2021

OVS HWOL: support multiple MCPs #125

Merged

pliurh closed this Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OVS HWOL: use generic plugin to write switchdev device configuration #106

OVS HWOL: use generic plugin to write switchdev device configuration #106

zshi-redhat commented Apr 11, 2021 •

edited

Loading

lgtm-com bot commented Apr 11, 2021

zshi-redhat commented Apr 11, 2021

Mmduh-483 left a comment

Mmduh-483 Apr 12, 2021

zshi-redhat Apr 12, 2021

zshi-redhat Apr 13, 2021

Mmduh-483 Apr 12, 2021

zshi-redhat Apr 12, 2021

Mmduh-483 Apr 12, 2021

zshi-redhat Apr 12, 2021

Mmduh-483 Apr 12, 2021

zshi-redhat Apr 13, 2021

Mmduh-483 Apr 12, 2021

zshi-redhat Apr 12, 2021

zshi-redhat Apr 13, 2021

zshi-redhat commented Apr 12, 2021

Mmduh-483 commented Apr 13, 2021

zshi-redhat commented Apr 15, 2021

pliurh Apr 15, 2021

zshi-redhat Apr 15, 2021

zshi-redhat Apr 19, 2021

pliurh Apr 15, 2021

zshi-redhat Apr 15, 2021 •

edited

Loading

zshi-redhat Apr 15, 2021

pliurh Apr 16, 2021

zshi-redhat Apr 19, 2021

pliurh Apr 15, 2021

zshi-redhat Apr 15, 2021

pliurh Apr 15, 2021

zshi-redhat Apr 15, 2021

zshi-redhat Apr 19, 2021

pliurh Apr 16, 2021

zshi-redhat Apr 19, 2021

OVS HWOL: use generic plugin to write switchdev device configuration #106

OVS HWOL: use generic plugin to write switchdev device configuration #106

Conversation

zshi-redhat commented Apr 11, 2021 • edited Loading

lgtm-com bot commented Apr 11, 2021

zshi-redhat commented Apr 11, 2021

Mmduh-483 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

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

zshi-redhat commented Apr 12, 2021

Mmduh-483 commented Apr 13, 2021

zshi-redhat commented Apr 15, 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

zshi-redhat Apr 15, 2021 • 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

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

zshi-redhat commented Apr 11, 2021 •

edited

Loading

zshi-redhat Apr 15, 2021 •

edited

Loading