fpga: make admission webhook mode-less #358

rojkov · 2020-04-06T12:01:59Z

closes #301

bart0sh · 2020-04-09T10:57:04Z

@rojkov looks good to me so far. What about kustomise yamls? Do they need to be updated?

@kad can you review this PR please?

rojkov · 2020-04-09T11:10:01Z

@rojkov looks good to me so far. What about kustomise yamls? Do they need to be updated?

Yes. There's #318 for that.

rojkov · 2020-04-14T13:33:30Z

@kad ping. Could you please review the updates to README.md at least?

kad

Small nits, otherwise LGTM

kad · 2020-04-14T16:37:00Z

deployments/fpga_admissionwebhook/af-crd.yaml

@@ -20,3 +20,9 @@ spec:
            afuId:
              type: string
              pattern: '^[0-9a-f]{8,128}$'


nit: this might be overkill. we don't expect such long UUIDs.

Limited to 32.

kad · 2020-04-14T16:40:20Z

cmd/fpga_admissionwebhook/README.md

+is the first 31 characters of the region interface ID for Arria10 with DCP1.1
+firmware. The next 32 characters (`d8424dc4a4a3c413f89e433683f9040b`) is an accelerator function ID.
+
+The same mapping, but with its mode field set to `orchestrated`, translates


nit: "orchestrated" might be not the best choice of the terminology here for cluster admin UX, but I don't have right now better word. Also, it might be good idea to accept "pre-programmed" as alias to "programmed ?

Mm.. dunno. "programmed" can be read as "just in time programmed" aka orchestrated.
Maybe "static" and "dynamic"?

Also, it might be good idea to accept "pre-programmed" as alias to "programmed"

(This meant to say accept both pre-programmed and preprogrammed?)

Maybe "static" and "dynamic"

this would work, IMO, but it might be good to start using "programming mode" in documentation where we just say only "mode" today.

I've checked the docs, there's no place where just "mode" is mentioned. But in fpga_plugin's README.md af and preprogrammed are used interchangeably. The same with region and orchestrated. And "orchestration programmed" is an alias for orchestrated.

Probably we could rename pregrogrammed and af to static; orchestrated and region - to dynamic; regiondevel - to just devel. Also I'd avoid accepting aliases.

Should we keep using 'region' and 'af' modes in order not to break compatibility with the currently used plugin modes?

@bart0sh Yes, that's correct. I've added 03b815c. Could you check if it's clearer now?

Yes, it's correct now. Thank you.

@rojkov As for mode name I think we can continue using 'af' and 'region' as we got rid of webhook mode names 'preprogrammed' and 'orchestrated'. Although webhook names look better in my opinion, using plugin names would help us to avoid possible user confusions.

Alright, I'll stick to the plugin mode names everywhere. Will update the PR.

good idea, +1

kad · 2020-04-14T16:41:32Z

cmd/fpga_admissionwebhook/README.md

@@ -138,13 +120,31 @@ $ ./scripts/webhook-deploy.sh --ca-bundle-path /var/run/kubernetes/server-ca.crt

 # Mappings

-Requested FPGA resources are translated to AF resources. For example,
-`fpga.intel.com/arria10.dcp1.1-nlb0` is translated to `fpga.intel.com/af-d8424dc4a4a3c413f89e433683f9040b`.
+For the following mapping


It might be good to say a sentence or two, kind of "introduction", that mappings are essential part of the setup and give cluster administrator flexible instrument for managing FPGA bitstreams and access control for them?

Thanks! added

mythi

needs rebase

mythi · 2020-04-21T05:01:00Z

cmd/fpga_admissionwebhook/README.md

+is the first 31 characters of the region interface ID for Arria10 with DCP1.1
+firmware. The next 32 characters (`d8424dc4a4a3c413f89e433683f9040b`) is an accelerator function ID.
+
+The same mapping, but with its mode field set to `orchestrated`, translates


Also, it might be good idea to accept "pre-programmed" as alias to "programmed"

(This meant to say accept both pre-programmed and preprogrammed?)

Maybe "static" and "dynamic"

this would work, IMO, but it might be good to start using "programming mode" in documentation where we just say only "mode" today.

rojkov · 2020-04-21T07:57:44Z

Thanks! Rebased.

rojkov · 2020-04-22T08:08:42Z

Updated to use consistent mode names across the FPGA webhook and plugin.

bart0sh · 2020-04-22T09:58:49Z

I'm going to give other reviewers one day for their comments/approvals. Then I'll merge this.

bart0sh · 2020-04-22T10:01:12Z

@mythi @kad please review last time and approve/merge.

mythi · 2020-04-22T10:12:27Z

this looks good to me but should we squash commits?

kad · 2020-04-22T10:46:57Z

I agree with @mythi, let's squash and lgtm.

fpga: make AFU resource name 63 char long webhook: drop mode from README webhook: extend mappings description webhook: tighten CRD definitions webhook: drop mapping to non-existing afuId explicitly state mappings names can be in any format use consistent terminology across fpga webhook and plugin

bart0sh · 2020-04-23T09:20:55Z

@rojkov Plugin is failing in af mode due to incorrect socket file name:

> kubectl get pods
NAME                      READY   STATUS             RESTARTS   AGE
intel-fpga-plugin-rk69j   0/1     CrashLoopBackOff   4          2m23s

> kubectl logs intel-fpga-plugin-rk69j
E0423 09:13:28.819373       1 manager.go:125] Failed to serve fpga.intel.com/69528db6eb31577a8c3668f9faa081ff7df405cbd7acf7222f144b0b93acd18: listen unix /var/lib/kubelet/device-plugins/fpga.intel.com-69528db6eb31577a8c3668f9faa081ff7df405cbd7acf7222f144b0b93acd18.sock: bind: invalid argument
Failed to listen to plugin socket
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).setupAndServe
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:211
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).Serve
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:163
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*Manager).handleUpdate.func1
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/manager.go:123
runtime.goexit
	/usr/lib/golang/src/runtime/asm_amd64.s:1357

However, it seems to work in region mode.

rojkov · 2020-04-23T09:25:25Z

Thank you! I'll take a look.

…

On Thu, Apr 23, 2020 at 12:21 PM Ed Bartosh ***@***.***> wrote: @rojkov <https://github.com/rojkov> Plugin is failing in af mode due to incorrect socket file name: > kubectl get pods NAME READY STATUS RESTARTS AGE intel-fpga-plugin-rk69j 0/1 CrashLoopBackOff 4 2m23s > kubectl logs intel-fpga-plugin-rk69j E0423 09:13:28.819373 1 manager.go:125] Failed to serve fpga.intel.com/69528db6eb31577a8c3668f9faa081ff7df405cbd7acf7222f144b0b93acd18: listen unix /var/lib/kubelet/device-plugins/fpga.intel.com-69528db6eb31577a8c3668f9faa081ff7df405cbd7acf7222f144b0b93acd18.sock: bind: invalid argument Failed to listen to plugin socketgithub.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).setupAndServe /intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:211github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).Serve /intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:163github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*Manager).handleUpdate.func1 /intel-device-plugins-for-kubernetes/pkg/deviceplugin/manager.go:123 runtime.goexit /usr/lib/golang/src/runtime/asm_amd64.s:1357 However, it seems to work in region mode. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#358 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAIEYOSQTNR47QPTAHPTVTROACANANCNFSM4MCGZACQ> .

rojkov · 2020-04-23T11:20:43Z

Failed to listen to plugin socket

Too bad unix socket addresses can be 108 chars long in Linux. /var/lib/kubelet/device-plugins/fpga.intel.com-69528db6eb31577a8c3668f9faa081ff7df405cbd7acf7222f144b0b93acd18.sock is 115 chars.

Should I strip the interface ID part even further from 31 to 16? Are we ok with that?

bart0sh · 2020-04-23T11:23:55Z

@rojkov Do you think 16 chars would make unique enough id? Any other ways to solve this?

rojkov · 2020-04-24T09:39:34Z

The limit is not 115, but 108 (including a trailing zero I presume), so the practical limit, as I tested, is 107. I've just updated the PR. Could you please check it once more?

bart0sh · 2020-04-24T09:51:12Z

pkg/fpga/devtypes.go

+		return "", errors.Wrapf(err, "failed to decode %q and %q", interfaceID, afuID)
+	}
+
+	return fmt.Sprintf("%s.%s.%s", interfaceID[:3], afuID[:3], base64.RawURLEncoding.EncodeToString(bin)), nil


Can we have 4 characters long prefixes and '-' delimiters?

Unlike dots '-' can be in the encoded section, so dots do a better job at separating the sections visually imho.
But ok, will update.

True. Feel free to use dots then. Just increase prefixes to 4 chars.

bart0sh · 2020-04-24T09:56:44Z

The limit is not 115, but 108

We can still use 4 chars long prefixes, right?

In [1]: len('/var/lib/kubelet/device-plugins/fpga.intel.com-') + len('ce48-d842-zkiWk5jwXzOUbVYHCL4QithCTcSko8QT-J5DNoP5BAs') + len('.sock')                                      
Out[1]: 105

bart0sh · 2020-04-24T12:59:37Z

/LGTM

I'll run e2e tests and report here if it works or not.

@mythi @kad please review this PR again. Hopefully last time.

bart0sh · 2020-04-24T14:02:27Z

@rojkov Could it be that mappings are setup incorrectly?

When I run plugin in af mode the node has these af resources:

Allocatable:

  fpga.intel.com/6952.d842.aVKNtusxV3qMNmj5-qCB9thCTcSko8QT-J5DNoP5BAs:  1
  fpga.intel.com/6952.f7df.aVKNtusxV3qMNmj5-qCB9vffQFy9es9yIvFEsLk6zRg:  1

However, the pod resource requests are mutated to region- resources:

> cat <<END | kubectl create -f-
> apiVersion: v1
> kind: Pod
> metadata:
>   name: test-nlb0
> spec:
>   containers:
>   - name: test-nlb0
>     image: intel/opae-nlb-demo:devel
>     imagePullPolicy: IfNotPresent
>     command: ['nlb0']
>     securityContext:
>       capabilities:
>         add:
>           [IPC_LOCK]
>     resources:
>       limits:
>         fpga.intel.com/arria10.dcp1.2-nlb0: 1
>         cpu: 1
>         hugepages-2Mi: 20Mi
> 
>   restartPolicy: Never
> END
pod/test-nlb0 created

> kubectl describe pod test-nlb0 |grep -A10 Limits
    Limits:
      cpu:                                                     1
      fpga.intel.com/region-69528db6eb31577a8c3668f9faa081f6:  1
      hugepages-2Mi:                                           20Mi
    Requests:
      cpu:                                                     1
      fpga.intel.com/region-69528db6eb31577a8c3668f9faa081f6:  1
      hugepages-2Mi:                                           20Mi
    Environment:
      FPGA_AFU_1:     d8424dc4a4a3c413f89e433683f9040b
      FPGA_REGION_1:  69528db6eb31577a8c3668f9faa081f6

Side note - we may want to add 'af-' prefix to af resource names to make the resource naming consistent. We can do that by reducing af id and interface id prefixes to 3 chars each.

rojkov · 2020-04-24T14:23:17Z

Could it be that mappings are setup incorrectly?

The arria10.dcp1.2-nlb0 mapping is configured to translate to region mode in deployments/fpga_admissionwebhook/mappings-collection.yaml. Just update the mapping.

I can add one more mapping arria10.dcp.1.2-nlb0-preprogrammed for testing purposes to the collection.

Regarding the af- prefix. Yes, will add.

bart0sh · 2020-04-24T15:46:38Z

I can add one more mapping arria10.dcp.1.2-nlb0-preprogrammed for testing purposes to the collection.

Yes, please do. I'd also suggest renaming arria10.dcp.1.2-nlb0 to arria10.dcp.1.2-nlb0-orchestrated to be more explicit.

rojkov · 2020-04-27T07:39:02Z

Ok, added. Also I've added -orchestrated suffix to all mappings with region mode for consistency.

bart0sh · 2020-04-27T15:38:33Z

@rojkov It's still failing. Looks like 4 symbols for afu and interface ids is too much:

> kubectl logs intel-fpga-plugin-87nrj --namespace fpgaplugin-e2e-8081
E0427 15:34:01.842986       1 manager.go:125] Failed to serve fpga.intel.com/af-6952.f7df.aVKNtusxV3qMNmj5-qCB9vffQFy9es9yIvFEsLk6zRg: listen unix /var/lib/kubelet/device-plugins/fpga.intel.com-af-6952.f7df.aVKNtusxV3qMNmj5-qCB9vffQFy9es9yIvFEsLk6zRg.sock: bind: invalid argument
Failed to listen to plugin socket
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).setupAndServe
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:211
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*server).Serve
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/server.go:163
github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin.(*Manager).handleUpdate.func1
	/intel-device-plugins-for-kubernetes/pkg/deviceplugin/manager.go:123
runtime.goexit
	/usr/lib/golang/src/runtime/asm_amd64.s:1357

bart0sh · 2020-04-27T15:39:15Z

@rojkov I'll try to reduce length of ids to 3. Hopefully this should be enough.

bart0sh · 2020-04-27T16:20:11Z

@rojkov worked for me with this change:

diff --git a/pkg/fpga/devtypes.go b/pkg/fpga/devtypes.go
index 87bbc44..5fc2b8f 100644
--- a/pkg/fpga/devtypes.go
+++ b/pkg/fpga/devtypes.go
@@ -32,5 +32,5 @@ func GetAfuDevType(interfaceID, afuID string) (string, error) {
                return "", errors.Wrapf(err, "failed to decode %q and %q", interfaceID, afuID)
        }
 
-       return fmt.Sprintf("af-%s.%s.%s", interfaceID[:4], afuID[:4], base64.RawURLEncoding.EncodeToString(bin)), nil
+       return fmt.Sprintf("af-%s.%s.%s", interfaceID[:3], afuID[:3], base64.RawURLEncoding.EncodeToString(bin)), nil
 }

Please, update the PR.

P.S. I'll send e2e test update as a PR to your branch rojkov:webhook-modeless

bart0sh · 2020-04-27T16:29:45Z

done: rojkov#5

rojkov · 2020-04-28T07:13:27Z

@rojkov It's still failing. Looks like 4 symbols for afu and interface ids is too much:

Oops, my bad. Reduced to 3 symbols now and cherry-picked your commit. Could you please try again?

bart0sh · 2020-04-28T10:51:41Z

@rojkov Thank you for the update! e2e tests worked!

@mythi @kad please review/approve/merge. Only you two can do that.

mythi · 2020-04-29T03:43:04Z

pkg/apis/fpga.intel.com/v1/types.go

 type AcceleratorFunctionSpec struct {
-	AfuID string `json:"afuId"`
+	AfuID       string `json:"afuId"`
+	InterfaceID string `json:"interfaceId"`
+	Mode        string `json:"mode"`
 }


What do you think about the need for v2 API version?

I suppose in this case we'd need to maintain two versions of the webhook: "mode-ful" and "mode-less". Making the webhook modeless is already a breaking change.

Making the webhook modeless is already a breaking change.

I was thinking v2 because of the breakage but did not think of the need to have both versions maintained (beyond what we have in the release branches).

Right. That makes sense if there are installed old mappings in a cluster. Will update the version on Monday.

@kad @bart0sh your thoughts?

I moved the fpga.intel.com API group to v2. It increased the size of the patch, but all tests pass.

mythi · 2020-04-29T03:59:28Z

cmd/fpga_admissionwebhook/README.md

+The same mapping, but with its mode field set to `region`, translates
+`fpga.intel.com/arria10.dcp1.1-nlb0` to `fpga.intel.com/region-9926ab6d6c925a68aabca7d84c545738`,


I wonder if it makes the reader confused when we use the same resource name as the example for both modes. Perhaps add -orchestrated suffix here as it's in the default CRDs.

Good catch! I forgot to reflect the compressed resource name in README.
Fixed now and used a real mapping from mappings-collection.yaml for the example.

Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>

cmd/fpga_admissionwebhook/README.md

rojkov requested review from bart0sh, kad and mythi as code owners April 6, 2020 12:01

rojkov force-pushed the webhook-modeless branch 2 times, most recently from a904596 to ec784d2 Compare April 8, 2020 07:10

rojkov mentioned this pull request Apr 9, 2020

kustomise doesn't work for admission controller webhook in preprogrammed mode #361

Closed

mythi mentioned this pull request Apr 10, 2020

implement e2e tests for FPGA plugin #359

Merged

kad previously approved these changes Apr 14, 2020

View reviewed changes

rojkov dismissed kad’s stale review via e387f4d April 15, 2020 12:01

rojkov force-pushed the webhook-modeless branch from e387f4d to 6b29065 Compare April 16, 2020 09:05

mythi reviewed Apr 21, 2020

View reviewed changes

rojkov force-pushed the webhook-modeless branch from 6b29065 to 10054b0 Compare April 21, 2020 07:48

rojkov force-pushed the webhook-modeless branch from 325dea1 to dbf5a35 Compare April 22, 2020 08:08

bart0sh previously approved these changes Apr 22, 2020

View reviewed changes

rojkov dismissed bart0sh’s stale review via 6c2eacf April 22, 2020 10:55

rojkov force-pushed the webhook-modeless branch from dbf5a35 to 6c2eacf Compare April 22, 2020 10:55

bart0sh reviewed Apr 24, 2020

View reviewed changes

rojkov force-pushed the webhook-modeless branch from 76c9c0e to cf2acb8 Compare April 24, 2020 10:59

rojkov force-pushed the webhook-modeless branch from cf2acb8 to bafc207 Compare April 27, 2020 07:34

rojkov force-pushed the webhook-modeless branch from 2bc66d6 to c15734f Compare April 28, 2020 07:11

bart0sh previously approved these changes Apr 28, 2020

View reviewed changes

mythi reviewed Apr 29, 2020

View reviewed changes

rojkov dismissed bart0sh’s stale review via 8d6afae April 29, 2020 08:59

rojkov force-pushed the webhook-modeless branch from c15734f to 8d6afae Compare April 29, 2020 08:59

Dmitry Rozhkov and others added 2 commits April 29, 2020 11:59

fpga: compress fpga AF resource names

99fcb69

fpga: update e2e tests to work with mode-less webhook

8d6afae

Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>

rojkov mentioned this pull request Apr 29, 2020

Admission webhook can not map to correct AFU ID #373

Closed

mythi reviewed Apr 29, 2020

View reviewed changes

cmd/fpga_admissionwebhook/README.md Show resolved Hide resolved

fpgawebhook: move to v2 API of fpga.intel.com group

c63dbf6

mythi approved these changes May 5, 2020

View reviewed changes

bart0sh approved these changes May 5, 2020

View reviewed changes

bart0sh merged commit 8b429fd into intel:master May 5, 2020

rojkov deleted the webhook-modeless branch June 25, 2020 14:22

		The same mapping, but with its mode field set to `region`, translates
		`fpga.intel.com/arria10.dcp1.1-nlb0` to `fpga.intel.com/region-9926ab6d6c925a68aabca7d84c545738`,

fpga: make admission webhook mode-less #358

fpga: make admission webhook mode-less #358

Conversation

rojkov commented Apr 6, 2020

bart0sh commented Apr 9, 2020

rojkov commented Apr 9, 2020

rojkov commented Apr 14, 2020

kad 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

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

Choose a reason for hiding this comment

rojkov commented Apr 21, 2020

rojkov commented Apr 22, 2020

bart0sh commented Apr 22, 2020

bart0sh commented Apr 22, 2020

mythi commented Apr 22, 2020

kad commented Apr 22, 2020

bart0sh commented Apr 23, 2020

rojkov commented Apr 23, 2020 via email

rojkov commented Apr 23, 2020

bart0sh commented Apr 23, 2020

rojkov commented Apr 24, 2020

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 Apr 24, 2020

bart0sh commented Apr 24, 2020

bart0sh commented Apr 24, 2020

rojkov commented Apr 24, 2020

bart0sh commented Apr 24, 2020

rojkov commented Apr 27, 2020

bart0sh commented Apr 27, 2020 • edited Loading

bart0sh commented Apr 27, 2020

bart0sh commented Apr 27, 2020

bart0sh commented Apr 27, 2020

rojkov commented Apr 28, 2020

bart0sh commented Apr 28, 2020

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 Apr 27, 2020 •

edited

Loading