tests_l2: Added qat workload #117

vbedida79 · 2023-08-30T19:06:07Z

tests_l2: Add qatlib based Intel QAT sample workload

qatlib is built from source in https://github.com/intel/qatlib
The qatlib version is specified by QATLIB_VERSION: 23.08.0
cpa_sample_code shipped with qatlib is used to verify the QAT provisioning on OCP
Resources:
- qat.intel.com/dc is used for QAT compression/decompression
- qat.intel.com/cy is used for QAT asymmetric/asymmetric cryptography
serviceAccount: intel-qat uses the user defined SCC intel-qat-scc which enables the IPC_LOCK capability for Qat-lib based workload see P1-Blocker: Can not run QAT workload in the non-priviledged container #122

Signed-off-by: veenadhari.bedida@intel.com

vbedida79 · 2023-08-30T19:10:04Z

@mregmi @uMartinXu please review

mythi · 2023-08-30T19:24:44Z

What's the reason it needs to run as privileged? It voids the plugin usage completely

hershpa · 2023-08-30T20:54:17Z

Good point @mythi. Is there a way we can address this? Via another fine grain SELinux policy?

vbedida79 · 2023-08-30T21:02:14Z

What's the reason it needs to run as privileged? It voids the plugin usage completely

test case is performing some vfio actions and fails with no device found. tried current container_device_t and combination of SCC's with volume mounts- don't seem to suffice either. right, @mregmi

mregmi · 2023-08-30T21:02:30Z

the workload needs access to vfio_device_t. So we have to add another permission to container_device_t. Until that is done either we have to create another custom policy for this workload or run selinux in permissive or use priviledge.

type=AVC msg=audit(1693324314.875:9260): avc: denied { read write } for pid=3334307 comm="cpa_sample_code" name="526" dev="devtmpfs" ino=46020946 scontext=system_u:system_r:container_t:s0:c19,c27 tcontext=system_u:object_r:vfio_device_t:s0 tclass=chr_file permissive=0

hershpa · 2023-08-30T21:07:14Z

We should track this as an known issue in a separate issue and submit a PR to add another permission to container_device_t for the workload to access vfio_device_t.

uMartinXu · 2023-08-31T19:51:17Z

cpa_sample_code

@vbedida79 could you file a github issue to track this permission problem?
I think this issue might be a blocker issue for the release. We can not simply take it as a known issue and do the release.

Looks like cpa_sample_code app tries to access devtempfs and got deneied. So we need to check whether it is the application that intends to access the devtempfs, or it is the qatlib to access devtempfs. If it is the qatlib access to the devtempfs, we have to update the container_device_t.
We have to do a full permission audit to check whether there still exists some other permission issues in qatlib, evaluate and add all the necessary permissions to container_device_t. Of course, it might also can be fixed in the qatlib. All in all, it should be transparent to our end-users who use qatlib and qat provisioning in the container on OCP, the privilege mode is absolutely not acceptable for the user workload running in a container.
If we do need to update the vfio_device_t, in order not to block the release, I think we need to activate and use https://github.com/intel/user-container-selinux. when we are working with RH and upstream to merge the new container_device_t into OCP. I don't know what is the earliest merge window for OCP.
@mregmi BTW, could you show us what the permissions configuration defined in vfio_device_t? Just from the name looks like we might need to add it for VFIO related provision.

mythi · 2023-09-01T05:01:24Z

Looks like cpa_sample_code app tries to access devtempfs and got deneied. So we need to check whether it is the application that intends to access the devtempfs, or it is the qatlib to access devtempfs. If it is the qatlib access to the devtempfs, we have to update the container_device_t.

Is DPDK "crypto-perf" the same? It would indeed be important to know if it's just cpa_sample_code doing something strange. I wish we had qatengine available for the builds. Testing openssl speed -t qatengine would also be a good data point.

On 2., agree but not just qatlib. I think we need someone focused on this and peek into DSA/IAA flows as well so we get the right settings in place early on.

mythi · 2023-09-06T17:23:52Z

tests/l2/qat/qatlib_build.yaml

+    dockerfile: |
+
+        FROM registry.access.redhat.com/ubi8/ubi AS builder
+        ARG QATLIB_VERSION="23.02.0"


Suggested change

ARG QATLIB_VERSION="23.02.0"

ARG QATLIB_VERSION="23.08.0"

it's not there yet but it would be good to move to it if it happens before your release date

23.08.0 has released, updated it

mythi · 2023-09-06T17:28:29Z

tests/l2/qat/qatlib_build.yaml

+              autoconf \
+              libtool \
+              openssl-devel \
+              http://mirror.centos.org/centos/8-stream/PowerTools/x86_64/os/Packages/nasm-2.15.03-3.el8.x86_64.rpm \


for the purpose of this build, I'd think we should go without nasm. See https://github.com/intel/qatlib/blob/39e19d4fc09345bb857ee8745619fa0f83f6f824/INSTALL#L224-L225

got it, thanks. will update

for the purpose of this build, I'd think we should go without nasm. See https://github.com/intel/qatlib/blob/39e19d4fc09345bb857ee8745619fa0f83f6f824/INSTALL#L224-L225

Looks like the option is for the situation nasm compiler is not present in the build environment. If we have the nasm compiler, we should install the nasm and build the lib with fast-crc-in-assembler, otherwise, there might be some performance penalty.

For this "test application", it's acceptable but pulling rpms from random urls is not.

Can look for official links of nasm. nasm & qatlib is on codeready-builder-for-rhel-8-x86_64-rpms repo for ubi images. With a dockerfile this repo can be accessed and built on RHEL systems enabled with subscription manager. Via buildconfigs, subscription manager and its supported repos can be enabled only via RH account personal keys. Plan to enable for future releases.

For this "test application", it's acceptable but pulling rpms from random urls is not.

@mythi I agree with your point. For the qatlib, the user should use RH distributed qatblib rpm package and shoul not build the lib by themselves. However, currently, it is not very easily for user to do that.The enhancement is filed for Milestone 1.1.0. to address the gap.

All in all, for the testing of the first QAT release(1.0.1), this test case is good enough.

@mythi Thank you very much for your comments and help.

vbedida79 · 2023-09-07T16:03:19Z

@uMartinXu @mythi @mregmi Updated PR. Please review.

mythi · 2023-09-07T16:11:50Z

LGTM!

vbedida79 · 2023-09-07T16:24:16Z

thanks for reviewing @mythi
@chaitanya1731 thanks for reviewing, made the changes to buildconfig. (remove ARG default, builder tag, 2 resources)

uMartinXu · 2023-09-07T17:31:29Z

tests/l2/qat/qatlib_rbac.yaml

+metadata:
+  annotations:
+    kubernetes.io/description: 'SCC to use IPC_LOCK capability for qatlib pod'
+  name: container-scc


Looks like we are creating a user-defined SCC for qat workload using qatlib. We should be very careful to create, maintain, and use a user-defined SCC see https://docs.openshift.com/container-platform/4.13/authentication/managing-security-context-constraints.html#security-context-constraints-creating_configuring-internal-oauth.
If we can't use the predefined SCC by OCP https://docs.openshift.com/container-platform/4.13/authentication/managing-security-context-constraints.html#default-sccs_configuring-internal-oauth
and have to create our own SCC. I suggest starting from predefined "restricted" or "restricted-v2" SCC and only adding the "IPC_LOCK" capability. To make the user-defined SCC more easily maintained, I suggest having a single yaml file to maintain it

Looks like we are creating a user-defined SCC for qat workload using qatlib.

It's for all containers using QAT through VFIO (also DPDK's libs and DPDK test apps like crypto/compress-perf belong to this)

Looks like we are creating a user-defined SCC for qat workload using qatlib. We should be very careful to create, I suggest starting from predefined "restricted" or "restricted-v2" SCC and only adding the "IPC_LOCK" capability. To make the user-defined SCC more easily maintained, I suggest having a single yaml file to maintain it

The pod needs to run as root, via restricted it wont work. Default capability with IPC_LOCK defaults to anyuid . The SCC here does not give access to file system and only executes as root with IPC_LOCK. Also, if other workloads need any other permissions, we just need to update this yaml to add it and in the job.

#35 is about making it possible to run containers as non-root if it helps to avoid custom SCCs

Thanks, tried this. For qatlib, it has a prereq to run as root https://github.com/intel/qatlib/blob/7429ee2b7c837137ed11959a3c2cc3729dc15739/INSTALL#L60. Will be following for gpu and sgx in 1.1.0 release

For qatlib, it has a prereq to run as root

@vbedida79 it's not true. At least in our orchestration case we can run QAT as non-root just like with gpu/sgx

Without root, sample code exits

qaeMemInit started dma_map_slab:200 VFIO_IOMMU_MAP_DMA failed va=7ffab563c000 iova=200000 size=200000 -- errno=12 ADF_UIO_PROXY err: adf_init_ring: unable to get ringbuf(v:(nil),p:(nil)) for rings in bank(0) ADF_UIO_PROXY err: icp_adf_transCreateHandle: adf_init_ring failed

https://github.com/intel/qatlib/blob/7429ee2b7c837137ed11959a3c2cc3729dc15739/INSTALL#L60 Any other setup/permission you suggest to deploy with?

if you wish to test it non-root, CRI-O must be configured with:

[crio.runtime] device_ownership_from_security_context = true

got it, on the host node then. this might be it. what do you think @mregmi and @uMartinXu

hershpa · 2023-09-12T18:56:31Z

LGTM!

uMartinXu · 2023-09-13T15:39:41Z

tests/l2/qat/qatlib_build.yaml

+
+        RUN dnf -y update && \
+            dnf install -y gcc \
+              systemd-devel \


do we really need systemd-devel?

Added from the dependencies according to qatlib doc https://github.com/intel/qatlib/blob/7429ee2b7c837137ed11959a3c2cc3729dc15739/INSTALL#L168

--enable-systemd=no and without systemd-devel is the right choice

@vbedida79 can we remove the systemd-devel package and figure out whether we still can build it. :-)

updated it, thanks

uMartinXu · 2023-09-13T15:40:48Z

tests/l2/qat/qatlib_build.yaml

+              ./autogen.sh && \
+              ./configure \
+              --prefix=/usr \
+              --disable-fast-crc-in-assembler \


Why do we need to disable-fast-crc-in-assembler?

Nasm is installed via rpmand not available via yum/dnf in ubi. used this option. as suggested here #117 (comment)

updated back to nasm, as discussed. @mregmi @uMartinXu please review

What was the justification to add it back?

@mregmi @uMartinXu mentioned possibility of performance issues with nasm removed

uMartinXu · 2023-09-13T19:00:31Z

tests/l2/qat/qatlib_build.yaml

@@ -0,0 +1,62 @@
+# Copyright (c) 2023 Intel Corporation


Could we change the commit log to:

tests_l2: Add qatlib based Intel QAT sample workload

qatlib is built from source in https://github.com/intel/qatlib
The qatlib version is specified by QATLIB_VERSION: 23.08.0
cpa_sample_code shipped with qatlib is used to verify the QAT provisioning on OCP

Resource:
qat.intel.com/dc is used for QAT compression/decompression
qat.intel.com/cy is used for QAT asymmetric/asymmetric cryptography

serviceAccount: intel-qat uses the user defined SCC intel-qat-scc which enables the IPC_LOCK capability for Qat-lib based workload see #122

Signed-off-by: veenadhari.bedida@intel.com

I think we should also add the detailed commit log in the git. The PR description can only be accessed from the online GitHub service. Correct me @mythi @vbedida79

can update commit message. do we plan to do this for every PR?

Yes, please. :-)

uMartinXu · 2023-09-13T19:02:19Z

tests/l2/qat/qatlib_job.yaml

+              [IPC_LOCK]
+        resources:
+          requests:
+            qat.intel.com/dc: '1'


Have we tried the case when we have more than one qat resource?

yes. /dev/vfio will have 4 dev files instead of 2. since 2 of dc and 2 of cy resources are requested

I mean have we ever tried to use multiple resources/VFs to do the dc or/and cy simultaneously in a single pod/container?

yes difference is what /dev/vfio would have. changed this job example, 2 cy and 2 dc. anything else to be checked here?

The provisioning of multiple resources works.
But we might need to figure out whether the multiple resources can actually be used by the test case to do dc or/and cy offloading to QAT device simultaneously in a single pod/container.
For this PR, I think it is good enough to merge.

qatlib is built from source in https://github.com/intel/qatlib The qatlib version is specified by QATLIB_VERSION: 23.08.0 cpa_sample_code shipped with qatlib is used to verify the QAT provisioning on OCP Resources: qat.intel.com/dc is used for QAT compression/decompression qat.intel.com/cy is used for QAT asymmetric/asymmetric cryptography serviceAccount: intel-qat uses the user defined SCC intel-qat-scc which enables the IPC_LOCK capability for Qat-lib based workload see intel#122 Signed-off-by: vbedida79 <veenadhari.bedida@intel.com>

Resolves intel#122 qatlib scc for IPC_LOCK in security/ qatlib needs to run as root SCC based on OCP default restricted-v2 SCC, with root permissions Used for intel#117 Signed-off-by: vbedida79 <veenadhari.bedida@intel.com>

vbedida79 force-pushed the patch-290823-1 branch from 150f817 to 1d041e1 Compare August 30, 2023 19:08

hershpa added the qat QAT feature label Aug 30, 2023

hershpa added this to the v1.0.1 milestone Aug 30, 2023

hershpa added the enhancement New feature or request label Aug 30, 2023

mythi reviewed Sep 6, 2023

View reviewed changes

vbedida79 force-pushed the patch-290823-1 branch 2 times, most recently from f07d2ce to 7211bff Compare September 7, 2023 15:54

vbedida79 force-pushed the patch-290823-1 branch 2 times, most recently from 8cf127d to 416af6c Compare September 7, 2023 16:22

uMartinXu reviewed Sep 7, 2023

View reviewed changes

vbedida79 force-pushed the patch-290823-1 branch 4 times, most recently from 449b8f6 to 018afe7 Compare September 12, 2023 16:36

This was referenced Sep 12, 2023

Security: Adding intel-qat-scc for Intel QAT based workload #132

Merged

tests_l2: Update readme #133

Merged

uMartinXu reviewed Sep 13, 2023

View reviewed changes

vbedida79 force-pushed the patch-290823-1 branch 2 times, most recently from fb21cbd to 20ae597 Compare September 13, 2023 21:01

vbedida79 force-pushed the patch-290823-1 branch from 20ae597 to 730ba89 Compare September 13, 2023 22:40

uMartinXu merged commit 573e29e into intel:main Sep 13, 2023

tests_l2: Added qat workload #117

tests_l2: Added qat workload #117

Conversation

vbedida79 commented Aug 30, 2023 • edited Loading

vbedida79 commented Aug 30, 2023

mythi commented Aug 30, 2023

hershpa commented Aug 30, 2023 • edited Loading

vbedida79 commented Aug 30, 2023 • edited Loading

mregmi commented Aug 30, 2023

hershpa commented Aug 30, 2023 • edited Loading

uMartinXu commented Aug 31, 2023 • edited Loading

mythi commented Sep 1, 2023

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

uMartinXu Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

vbedida79 commented Sep 7, 2023

mythi commented Sep 7, 2023

vbedida79 commented Sep 7, 2023 • edited Loading

uMartinXu Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbedida79 Sep 8, 2023 • 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

vbedida79 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hershpa commented Sep 12, 2023

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

vbedida79 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbedida79 commented Aug 30, 2023 •

edited

Loading

hershpa commented Aug 30, 2023 •

edited

Loading

vbedida79 commented Aug 30, 2023 •

edited

Loading

hershpa commented Aug 30, 2023 •

edited

Loading

uMartinXu commented Aug 31, 2023 •

edited

Loading

uMartinXu Sep 14, 2023 •

edited

Loading

vbedida79 commented Sep 7, 2023 •

edited

Loading

uMartinXu Sep 7, 2023 •

edited

Loading

vbedida79 Sep 8, 2023 •

edited

Loading

vbedida79 Sep 14, 2023 •

edited

Loading

vbedida79 Sep 13, 2023 •

edited

Loading