Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment #39

Mmduh-483 · 2021-01-07T06:17:50Z

This PR introduces new changes to support changing E-switch mode for vanilla Kubernetes

Introduce service package to maintain services on vanilla Kubernetes, this is needed as an alternative for Openshift MCO
Move bindata/manifests/machine-config to bindata/manifests/switchdev-config, to avoid confuion of Openshift MCO
Introduce Kubernetes plugin to maintain services for switchdev
Add documention for hardware offload

bindata/manifests/k8s-services/configure-switchdev.sh

moshe010 · 2021-01-25T19:21:19Z

pkg/service/utils.go

+}
+
+// ServiceAppend appends missing fields of serviceB into serviceA
+func ServiceAppend(serviceA, serviceB *Service) (*Service, error) {


I think you can change to ServiceAppend(serviceA *Service, contentToAppend)((*Service, error)
Why do you need the serviceB?

Also maybe change to AppendToService to be same as RemoveFromService

moshe010 · 2021-01-25T19:21:41Z

pkg/service/utils.go

+	return false, nil
+}
+
+// CompareServices compare 2 service and return true if serviceA has all the fields of serviceB


update comment

moshe010 · 2021-01-25T19:27:08Z

pkg/service/utils.go

+			}
+		}
+
+		if !found {


so after reading about the break label I think what you did first with the outer was better to break from both loops.

moshe010 · 2021-01-25T19:28:51Z

pkg/service/service_manager.go

+	return cmd.Run()
+}
+
+func (sm *serviceManager) isServiceLoaded(servicePath string) (bool, error) {


What do you mean by loaded?
Do you mean if the Service is activate? start? and why checking the file is good enough ?

moshe010 · 2021-01-25T19:30:00Z

pkg/service/service_manager.go

+}
+
+// DisableService disables service with systemctl disable and removes service file
+func (sm *serviceManager) DisableService(service *Service) error {


are you using all the serviceManager apis? if not just keep the ones that you are currently use.

moshe010 · 2021-01-25T19:32:21Z

pkg/plugins/k8s/k8s_plugin.go

+
+	if updateTarget.networkManager {
+		nmService, err := serviceManager.ReadService(networkManagerService.Path)
+		if err != nil {


were is the ovs service?

moshe010 · 2021-01-26T08:29:15Z

pkg/plugins/k8s/k8s_plugin.go

+var (
+	Plugin K8sPlugin
+
+	serviceManager service.ServiceManager


moshe010 · 2021-01-27T06:44:22Z

pkg/plugins/k8s/k8s_plugin.go

+}
+
+const (
+	mcoManifestPath           = "bindata/manifests/machine-config/"


let rename the folder to be bindata/manifests/switchdev-config/
and MCO andK8s both of them will use it.

moshe010 · 2021-01-27T06:45:08Z

pkg/plugins/k8s/k8s_plugin.go

+	}
+}
+
+type McoServiceInjection struct {


remove all the Mco prefix from all the struct as they are not releated to MCO

moshe010 · 2021-01-27T06:46:33Z

bindata/manifests/daemon/daemonset.yaml

+        lifecycle:
+          preStop:
+            exec:
+              command: ["/bin/sh","-c","rm -f /host/etc/systemd/system/switchdev-configuration.service"]


this is not good enough. We should undo all the other changes

pkg/plugins/k8s/k8s_plugin.go

moshe010 · 2021-01-27T06:53:31Z

pkg/plugins/k8s/k8s_plugin.go

+}
+
+func updateService(serviceObj *service.Service) error {
+	systemService, err := serviceManager.ReadService(serviceObj.Path)


if you will move the compare here you won't need to check if that service need update.
Just do it here.

updateService is called from Apply when there is change needed, the compare is running from OnNodeChange which check if there is change needed to be applied

moshe010 · 2021-01-31T13:23:13Z

bindata/scripts/clean-k8s-services.sh

+
+clean_services
+# Reload host services
+chroot $chroot_path /bin/bash -c systemctl daemon-reload > /dev/null


shouldn't you restart the ovs and NM services?

moshe010 · 2021-01-31T13:25:52Z

pkg/plugins/k8s/k8s_plugin.go

+		updateList = append(updateList, "SwitchdevService")
+	}
+	if u.switchdevScript {
+		updateList = append(updateList, "SwitchdevService")


should be "switchdevScript"

moshe010 · 2021-01-31T13:28:20Z

pkg/plugins/k8s/k8s_plugin.go

+
+	p.updateTarget.reset()
+	// Check services
+	err = p.isServicesNeedUpdate()


isServicesNeedUpdate should return bool not err

moshe010 · 2021-01-31T13:30:13Z

pkg/plugins/k8s/k8s_plugin.go

+	return nil
+}
+
+func (p *K8sPlugin) readSystemServiceManifestFile(path string) (*service.Service, error) {


This should move to be in the function ServiceManager file like the compare services

This is reading from bindata/manifests/machine-config and parsing the yaml file, I don't think this is related to ServiceManager
WDYT?

Why not? because it end with yaml? I think in the future when we will add services if any it will be in the same approach. Don't you think?

moshe010 · 2021-02-02T06:54:23Z

pkg/plugins/k8s/k8s_plugin.go

+	return nil
+}
+
+func (p *K8sPlugin) readManifestFiles() error {


it better to read and inject for each service so network manager and ovs should have there own method.
so you have one method of NM and one for ovs which does the read and inject

pkg/plugins/k8s/k8s_plugin.go

moshe010 · 2021-02-02T07:00:30Z

pkg/plugins/k8s/k8s_plugin.go

+	return []*service.Service{p.networkManagerService, p.openVSwitchService}
+}
+
+func (p *K8sPlugin) isSystemServiceNeedUpdate(serviceObj *service.Service) (bool, error) {


let split the logic.
You should have isSystemServiceExist() to validate at start
and then isSystemServiceNeedUpdate can return just bool

pkg/plugins/k8s/k8s_plugin.go

moshe010 · 2021-02-02T23:22:09Z

pkg/plugins/k8s/k8s_plugin.go

+	}
+	if p.updateTarget.needUpdate() {
+		glog.Infof("k8s-plugin OnNodeStateChange(): needDrain to update %q", p.updateTarget)
+		needDrain = true


don't you need reboot?

Adding the services doesn't require reboot

but what about the VF rep udev rename? it won't be triggered ...

It is possible to add an additional condition to the udev renaming to do reboot, but the PR of renaming is not merged yet

Sorry I don't follow why it better to do it in the udev PR and not here?

I assume would be better because the udev is not merged yet, and when udev is merged we need to do another PR to support it for k8s_plugin

pkg/plugins/k8s/k8s_plugin.go

moshe010 · 2021-02-02T23:26:18Z

pkg/plugins/k8s/k8s_plugin.go

+			return err
+		}
+		if !exist {
+			continue


if it not exist it a problem we should log error

if it not exist it means the NetworkManager or ovs is not installed on the host so a warning would serve better unless you consider them a pre-request, WDYT?

The ovs is must not sure about the NM

So it would be better to split the logic to check if the service needs an update because there are mandatory and optional services

Let assume all of are mandatory ovs and NM

moshe010 · 2021-02-03T19:48:10Z

pkg/plugins/k8s/k8s_plugin.go

+	}
+	if p.updateTarget.needUpdate() {
+		glog.Infof("k8s-plugin OnNodeStateChange(): needDrain to update %q", p.updateTarget)
+		needDrain = true


Sorry I don't follow why it better to do it in the udev PR and not here?

moshe010 · 2021-02-03T19:49:50Z

pkg/utils/switchdev.go

+	}
+	update = true
+	glog.V(2).Infof("WriteSwitchdevConfFile(): write %s to switchdev.conf", newContent)
+	err = ioutil.WriteFile(switchDevConfPath, []byte(newContent), 0666)


I assume this what the permission in the MCO commit. I think it better to change it to 644.
@zshi-redhat what do you thing?

Yes, we only need this file be readable for users. mco/k8s plugins write the file.

martinkennelly · 2021-02-04T17:14:40Z

/LGTM - Can you document the feature to advertise it? I am not sure where, features in readme? - Maybe we need a switchdev doc.

zshi-redhat · 2021-02-08T02:09:43Z

/LGTM - Can you document the feature to advertise it? I am not sure where, features in readme? - Maybe we need a switchdev doc.

@Mmduh-483 would you mind write a section in doc/ovs-hw-offload.md for using switchdev device on k8s? Thanks!

Mmduh-483 · 2021-02-09T13:27:36Z

@moshe010 @zshi-redhat @martinkennelly added docs under doc/ovs-hw-offload.md

moshe010 · 2021-02-09T20:10:12Z

doc/ovs-hw-offload.md

+host net-device. The VF representor plays the same role as TAP devices
+in Para-Virtual (PV) setup. A packet sent through the VF representor on the host
+arrives to the VF, and a packet sent through the VF is received by its representor.
+


we should have prerequisites section:

ovs installed

Network Manager installed

doc/ovs-hw-offload.md

moshe010

@martinkennelly can you please take a look again. documentation was added

moshe010 · 2021-02-19T18:27:32Z

@zshi-redhat Can we merge this ?

pliurh

If I read the code correctly, the k8s plugin will always try to update the systemd services even when switchdev is not configured. Did I miss anything?

pliurh · 2021-02-22T07:02:24Z

bindata/manifests/daemon/daemonset.yaml

+        lifecycle:
+          preStop:
+            exec:
+              command: ["/bindata/scripts/clean-k8s-services.sh"]


Do we need to always run this script when the daemonset pod is terminated? At least, I don't think we need it for openshift.

Update the script to exist if CLUSTER_TYPE==openshift

Mmduh-483 · 2021-02-22T10:21:32Z

If I read the code correctly, the k8s plugin will always try to update the systemd services even when switchdev is not configured. Did I miss anything?

Updated to update services and swithdev config if policy is switchdev

pliurh · 2021-03-02T02:18:01Z

@Mmduh-483 Could you add more information to the commit description? This patch does more than just changing Eswitch mode. Also, I suggest adding a 'TODO' in the code, saying we will address the enableOvsOffload flag issue in the future.

Add pkg/service which contain utils to read MCO files for switchdev services and scripts, this package is needed because vanilla Kubernetes doesn't have support MCO which is part of openshift Signed-off-by: Mamduh Alassi <[email protected]>

…config Avoid confusion by machine-config which is related to Openshift MCO Signed-off-by: Mamduh Alassi <[email protected]>

Openshift is handling switchdev by mco_plugin which uses Openshift MCO to handle the services, this commit introduce k8s_plugin which handle the same logic for vanilla Kubernetes that is missing MCO Signed-off-by: Mamduh Alassi <[email protected]>

Signed-off-by: Mamduh Alassi <[email protected]>

Mmduh-483 · 2021-03-02T07:43:14Z

@Mmduh-483 Could you add more information to the commit description? This patch does more than just changing Eswitch mode. Also, I suggest adding a 'TODO' in the code, saying we will address the enableOvsOffload flag issue in the future.

Updated the PR with separated commits with detailed commit description for each commit and the PR description

Mmduh-483 force-pushed the switchdev-k8s branch from 5071499 to 7bc1804 Compare January 7, 2021 14:27

moshe010 reviewed Jan 10, 2021

View reviewed changes

bindata/manifests/k8s-services/configure-switchdev.sh Outdated Show resolved Hide resolved

Mmduh-483 force-pushed the switchdev-k8s branch from 7bc1804 to 0b32f7c Compare January 10, 2021 12:40

Mmduh-483 changed the title ~~[WIP] Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment~~ Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment Jan 10, 2021

Mmduh-483 force-pushed the switchdev-k8s branch from 0b32f7c to 9bb80db Compare January 17, 2021 07:33

Mmduh-483 changed the title ~~Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment~~ [WIP] Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment Jan 17, 2021

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 60a0192 to 9a50ee9 Compare January 18, 2021 10:51

Mmduh-483 changed the title ~~[WIP] Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment~~ Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment Jan 18, 2021

Mmduh-483 force-pushed the switchdev-k8s branch from 9a50ee9 to 0480bd1 Compare January 25, 2021 09:11

Mmduh-483 changed the title ~~Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment~~ [WIP] Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment Jan 25, 2021

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 5fa8518 to b863730 Compare January 25, 2021 16:18

moshe010 requested changes Jan 25, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 66035f1 to d6aebc4 Compare January 26, 2021 06:31

moshe010 requested changes Jan 27, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 0880741 to 357bdb2 Compare January 28, 2021 08:21

moshe010 requested changes Jan 31, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from 357bdb2 to 740b9bd Compare February 1, 2021 09:05

Mmduh-483 changed the title ~~[WIP] Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment~~ Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment Feb 1, 2021

Mmduh-483 requested a review from moshe010 February 1, 2021 09:05

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 5cbf7bd to 6a59056 Compare February 1, 2021 11:28

moshe010 requested changes Feb 2, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from 6a59056 to ee62864 Compare February 2, 2021 14:31

moshe010 reviewed Feb 2, 2021

View reviewed changes

moshe010 reviewed Feb 3, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from b78287a to 5c07954 Compare February 4, 2021 08:15

Mmduh-483 requested review from moshe010 and zshi-redhat February 4, 2021 08:16

moshe010 approved these changes Feb 4, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from 5c07954 to 250c487 Compare February 9, 2021 13:24

moshe010 reviewed Feb 9, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from 250c487 to e50e02f Compare February 10, 2021 08:42

moshe010 reviewed Feb 10, 2021

View reviewed changes

doc/ovs-hw-offload.md Outdated Show resolved Hide resolved

Mmduh-483 force-pushed the switchdev-k8s branch from e50e02f to 2afad89 Compare February 11, 2021 07:26

moshe010 approved these changes Feb 11, 2021

View reviewed changes

pliurh reviewed Feb 22, 2021

View reviewed changes

Mmduh-483 force-pushed the switchdev-k8s branch from 2afad89 to faf47f5 Compare February 22, 2021 10:19

Mmduh-483 force-pushed the switchdev-k8s branch 2 times, most recently from 71a4f56 to 5cbd2d5 Compare February 23, 2021 14:30

adrianchiris mentioned this pull request Feb 25, 2021

Looking for github release criteria #69

Closed

Mmduh-483 added 4 commits March 2, 2021 09:26

Move bindata/manifests/machine-config to bindata/manifests/switchdev-…

66f6f26

…config Avoid confusion by machine-config which is related to Openshift MCO Signed-off-by: Mamduh Alassi <[email protected]>

Add documention for hardware offload

5067405

Signed-off-by: Mamduh Alassi <[email protected]>

Mmduh-483 force-pushed the switchdev-k8s branch from 5cbd2d5 to 5067405 Compare March 2, 2021 07:38

pliurh merged commit e213aa2 into k8snetworkplumbingwg:master Mar 2, 2021

pliurh mentioned this pull request Mar 4, 2021

release 1.0.0 tracker #79

Closed

6 tasks

Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment #39

Support changing Eswitch mode of SR-IOV NICs for kubernetes deployment #39

Conversation

Mmduh-483 commented Jan 7, 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

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

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

martinkennelly commented Feb 4, 2021

zshi-redhat commented Feb 8, 2021

Mmduh-483 commented Feb 9, 2021

Choose a reason for hiding this comment

moshe010 left a comment

Choose a reason for hiding this comment

moshe010 commented Feb 19, 2021

pliurh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mmduh-483 commented Feb 22, 2021

pliurh commented Mar 2, 2021

Mmduh-483 commented Mar 2, 2021

Mmduh-483 commented Jan 7, 2021 •

edited

Loading