Make it possible to select supported devices in the operator #599

ozhuraki · 2021-03-15T15:35:43Z

In order to support per device deployment in the operator:

Add customization overlays for FPGA, DSA, SGX, GPU, QAT under deployments/operator/device/
Add --device cmdline to operator main.go which defines the controllers/webhooks to set up

mythi · 2021-03-15T16:58:28Z

mythi · 2021-03-15T19:10:49Z

cmd/operator/main.go

-	pm := patcher.NewPatcherManager(mgr.GetLogger().WithName("webhooks").WithName("Fpga"))
+	if deviceType == "fpga" {
+		pm := patcher.NewPatcherManager(mgr.GetLogger().WithName("webhooks").WithName("Fpga"))
+	}

 	mgr.GetWebhookServer().Register("/pods", &webhook.Admission{
 		Handler: admission.HandlerFunc(pm.GetPodMutator()),


this is also part of FPGA.

OK, thanks, moved it under "if".

mythi · 2021-03-15T19:26:16Z

deployments/operator/device/dsa/kustomization.yaml

@@ -0,0 +1,5 @@
+bases:
+ - ../../default


this also pulls in ../../crd/bases which creates the CRDs for all devices.

What would be a way to drop the unneeded CRDs?

I tried with the approach to pick only specific CRDs (for example for FPGA), however couldn't get it to work:

diff --git a/deployments/operator/device/fpga/kustomization.yaml b/deployments/operator/device/fpga/kustomization.yaml index cea26f2..60de267 100644 --- a/deployments/operator/device/fpga/kustomization.yaml +++ b/deployments/operator/device/fpga/kustomization.yaml @@ -1,5 +1,16 @@ +namespace: inteldeviceplugins-system +namePrefix: inteldeviceplugins- + +resources: +- deviceplugin.intel.com_fpgadeviceplugins.yaml +- fpga.intel.com_acceleratorfunctions.yaml +- fpga.intel.com_fpgaregions.yaml + bases: - - ../../default +- ../../rbac +- ../../manager +- ../../webhook +- ../../certmanager patchesStrategicMerge: - fpga.yaml

# kustomize build deployments/operator/device/fpga Error: no matches for IdId apps_v1_Deployment|inteldeviceplugins-system|inteldeviceplugins-controller-manager; failed to find unique target for patch apps_v1_Deployment|inteldeviceplugins-controller-manager

What would be a way to drop the unneeded CRDs?

maybe the only way to do it is to have the CRDs created by the manager itself. This could follow the idea I demonstrate in #394 (comment)

mythi · 2021-03-15T19:27:14Z

deployments/operator/device/dsa/dsa.yaml

+kind: Deployment
+metadata:
+  name: inteldeviceplugins-controller-manager
+  namespace: inteldeviceplugins-system


was the namespace required for this to work?

I have tried to omit it, however kustomize errors without namespace:

Error: no matches for IdId apps_v1_Deployment|~X|inteldeviceplugins-controller-manager; failed to find unique target for patch apps_v1_Deployment|inteldeviceplugins-controller-manager

bart0sh · 2021-03-17T12:56:44Z

deployments/operator/device/gpu/gpu.yaml

+      - args:
+        - --metrics-addr=127.0.0.1:8080
+        - --enable-leader-election
+        - --device=fpga


should it be --device=gpu?

bart0sh · 2021-03-17T12:57:45Z

deployments/operator/device/qat/qat.yaml

+      - args:
+        - --metrics-addr=127.0.0.1:8080
+        - --enable-leader-election
+        - --device=fpga


the same here, should be --device=qat I guess.

Thanks, good catch. Fixed both.

bart0sh · 2021-03-17T13:09:00Z

cmd/operator/main.go

 	flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
 	flag.StringVar(&devicePluginNamespace, "deviceplugin-namespace", metav1.NamespaceSystem, "The namespace where deviceplugin daemonsets are created")
 	flag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
+	flag.StringVar(&deviceType, "device", "", "Device type to set up.")


does this mean that only one device can be chosen?

Sure, we can allow multiple devices. However as kustmizations overlays are per device type, would it make sense, what do you think?

I don't know if this is possible, but it would be nice to support more than one device at a time. Otherwise if cluster has more than one device type users will have to use default operator with support of all devices.

OK, let's support also more then one device at a time.

It could be done so that -devices parameter is a comma separated list, you split it, validate and loop. fpga/sgx webhook and fpga crd controllers' registration could also be moved into the loop with if fpga {} and if sgx {}.

Sure, let's make it so.

I'd suggest a bit more user friendly approach:

package main import "fmt" import "flag" import "strings" type flagList []string func (fl *flagList) String() string { return strings.Join(*fl, ", ") } func (fl *flagList) Set(value string) error { *fl = append(*fl, value) return nil } var devices flagList func main() { flag.Var(&devices, "device", "device to enable, can be used multiple times") flag.Parse() fmt.Println(devices) }

sorry, forgot to show how it looks in the command line:

$ go run flags.go --device fpga --device gpu [fpga gpu]

OK, thanks, let's do so.

cmd/operator/main.go

README.md

cmd/operator/main.go

bart0sh · 2021-03-22T14:21:12Z

@ozhuraki There was some activity on combining components in kustomize last year: kubernetes-sigs/kustomize#1251 Would this approach help us to generate combined operator resources for multiple devices?

ozhuraki · 2021-03-22T15:01:02Z

@bart0sh

Would this approach help us to generate combined operator resources for multiple devices?

Thanks. It might help when we solve the problem of dropping the unwanted CRDs (at the moment they are combined).

I tried to pull only the required bases, however it didn't work (see the discussion above). One possibility is:

maybe the only way to do it is to have the CRDs created by the manager itself. This could follow the idea I demonstrate in #394 (comment)

bart0sh · 2021-03-23T09:01:51Z

cmd/operator/main.go

-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "FpgaRegion")
-		os.Exit(1)
+	if devices["fpga"] {


this if condition and above if devices["fpga"] can be merged into one. The same can be done for 2 if devices["sgx"] conditions.

OK, thanks! I will merge them.

ozhuraki · 2021-03-23T14:00:28Z

OK, updated, please take a look:

Added a validation of --device arguments
Added a check for duplicates
Switched to string array
Merged "fpga" clauses

bart0sh

/lgtm

bart0sh · 2021-03-31T09:16:55Z

@mythi @rojkov please review.

mythi · 2021-04-08T07:11:34Z

README.md

+$ kubectl apply -k deployments/operator/device/fpga
+```
+
+Operator also supports deployments with multiple selected device types.


Let's add a note that the CRDs are created but no controller is registered.

Also, please rebase. This section got moved to cmd/operator/README

@mythi

OK, thanks!

mythi

@ozhuraki sorry for the delay. I added a couple of comments.

marking as 'request changes' from me.

Add a patch to operator's manager.yaml to add "--device fpga" command line in orfer to enable per device deployment. Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

Add --device command line to operator's main.go which defines the controllers/webhooks to set up. Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

ozhuraki requested review from bart0sh, kad, mythi and rojkov as code owners March 15, 2021 15:35

mythi reviewed Mar 15, 2021

View reviewed changes

msivosuo mentioned this pull request Mar 16, 2021

make it possible to select supported devices in the operator #539

Open

4 tasks

ozhuraki force-pushed the operator-select-device-type branch 3 times, most recently from d5671cb to 546fca5 Compare March 16, 2021 16:42

bart0sh requested changes Mar 17, 2021

View reviewed changes

ozhuraki force-pushed the operator-select-device-type branch 5 times, most recently from 984b9cf to 112d660 Compare March 22, 2021 10:23

bart0sh reviewed Mar 22, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

bart0sh reviewed Mar 22, 2021

View reviewed changes

cmd/operator/main.go Show resolved Hide resolved

ozhuraki force-pushed the operator-select-device-type branch 3 times, most recently from f2e067f to f3bf258 Compare March 22, 2021 13:15

ozhuraki force-pushed the operator-select-device-type branch 4 times, most recently from 6a1fe7b to ce34ca9 Compare March 22, 2021 17:21

bart0sh reviewed Mar 23, 2021

View reviewed changes

intel deleted a comment from ozhuraki Mar 23, 2021

ozhuraki force-pushed the operator-select-device-type branch from ce34ca9 to 8f68925 Compare March 23, 2021 12:55

ozhuraki force-pushed the operator-select-device-type branch from 8f68925 to 95be9d1 Compare March 23, 2021 13:22

bart0sh previously approved these changes Mar 23, 2021

View reviewed changes

mythi reviewed Apr 8, 2021

View reviewed changes

mythi requested changes Apr 8, 2021

View reviewed changes

ozhuraki added 7 commits April 8, 2021 10:33

operator: fpga: Add a kustomization overlay for FPGA

1d68cbc

Add a patch to operator's manager.yaml to add "--device fpga" command line in orfer to enable per device deployment. Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: dsa: Add a kustomization overlay for DSA

de72b84

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: sgx: Add a kustomization overlay for SGX

9659f8c

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: gpu: Add a kustomization overlay for GPU

22a40d8

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: qat: Add a kustomization overlay for QAT

d5a1dda

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: Add --device command line to operator

2d27602

Add --device command line to operator's main.go which defines the controllers/webhooks to set up. Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

operator: README: Document per device deployment

6fbf7c9

Signed-off-by: Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>

ozhuraki dismissed bart0sh’s stale review via 6fbf7c9 April 8, 2021 10:54

ozhuraki force-pushed the operator-select-device-type branch from 95be9d1 to 6fbf7c9 Compare April 8, 2021 10:54

mythi approved these changes Apr 8, 2021

View reviewed changes

bart0sh approved these changes Apr 12, 2021

View reviewed changes

bart0sh merged commit 3161459 into intel:main Apr 12, 2021

ozhuraki deleted the operator-select-device-type branch December 17, 2021 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to select supported devices in the operator #599

Make it possible to select supported devices in the operator #599

ozhuraki commented Mar 15, 2021

mythi commented Mar 15, 2021

mythi Mar 15, 2021

ozhuraki Mar 16, 2021

mythi Mar 15, 2021

ozhuraki Mar 16, 2021

mythi Mar 18, 2021

mythi Mar 15, 2021

ozhuraki Mar 16, 2021

bart0sh Mar 17, 2021

bart0sh Mar 17, 2021

ozhuraki Mar 17, 2021

bart0sh Mar 17, 2021

ozhuraki Mar 17, 2021

bart0sh Mar 17, 2021

ozhuraki Mar 18, 2021

mythi Mar 18, 2021

ozhuraki Mar 18, 2021

bart0sh Mar 18, 2021

bart0sh Mar 18, 2021

ozhuraki Mar 18, 2021

mythi Mar 18, 2021

bart0sh commented Mar 22, 2021

ozhuraki commented Mar 22, 2021

bart0sh Mar 23, 2021

ozhuraki Mar 23, 2021

ozhuraki commented Mar 23, 2021

bart0sh left a comment

bart0sh commented Mar 31, 2021

mythi Apr 8, 2021

ozhuraki Apr 8, 2021

mythi left a comment

Make it possible to select supported devices in the operator #599

Make it possible to select supported devices in the operator #599

Conversation

ozhuraki commented Mar 15, 2021

mythi commented Mar 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

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

bart0sh commented Mar 22, 2021

ozhuraki commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozhuraki commented Mar 23, 2021

bart0sh left a comment

Choose a reason for hiding this comment

bart0sh commented Mar 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythi left a comment

Choose a reason for hiding this comment