Skip to content

qat: drop AppArmor annotations #1860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 16, 2025
Merged

qat: drop AppArmor annotations #1860

merged 3 commits into from
Jan 16, 2025

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Sep 30, 2024

It's been GA since k8s 1.30 so we can enable it unconditionally starting with our 0.32 release.

Fixes: #1818
Fixes: #1887

@mythi mythi force-pushed the PR-2024-022 branch 3 times, most recently from 84e99cb to d40efb3 Compare October 4, 2024 07:39
@mythi mythi marked this pull request as ready for review October 4, 2024 08:03
@mythi
Copy link
Contributor Author

mythi commented Oct 4, 2024

TODO: @hj-johannes-lee is still checking whether the vfio-pci.ids= cmdline settings can an alternative to Unconfined.

@hj-johannes-lee
Copy link
Contributor

hj-johannes-lee commented Oct 8, 2024

Unfortunately vfio-pci.ids= way does not alternate apparmor related matters.
There is no difference between having it or not.

@mythi
Copy link
Contributor Author

mythi commented Oct 18, 2024

Unfortunately vfio-pci.ids= way does not alternate apparmor related matters. There is no difference between having it or not.

I took a closer look. vfio-pci.ids= can help here but we are handling the new_id write errors a bit wrong. This function could just print a warning and not error so that the plugin fails.

@mythi mythi marked this pull request as draft October 18, 2024 12:46
@mythi mythi changed the title qat: set AppArmorProfile and drop AppArmor annotations qat: drop AppArmor annotations Oct 18, 2024
@mythi mythi force-pushed the PR-2024-022 branch 3 times, most recently from 5cabe2a to 2989f53 Compare January 15, 2025 08:52
@mythi mythi marked this pull request as ready for review January 15, 2025 08:52
@mythi mythi requested a review from kad as a code owner January 15, 2025 08:52
Copy link
Contributor

@hj-johannes-lee hj-johannes-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Tested both when vfio-pci.ids are enabled in cmdline and when not enabled.

mythi added 3 commits January 16, 2025 13:54
QAT device plugin has some initialization functions that require
special SecurityContext parameters (e.g., setting Apparmor policies
on some OSes).

It's better to move all of the initialization to the privileged
init-container that is already taking care some parts of it.

With this change, we default to vfio-pci "DpdkDrv".

Signed-off-by: Mikko Ylinen <[email protected]>
setupDeviceIDs() is obsoleted and the preferred approach is driver_override
already implemented in qat-init.sh initcontainer.

The new_id mechanism was added way before we had the initcontainer support in place.
Furthermore, at least for vfio-pci we don't need it at all if the driver uses
ids=8086:<qat VF dev IDs>.

Drop write attemps to new_id in favor of the initcontainer functionality.

Signed-off-by: Mikko Ylinen <[email protected]>
"unconfined" annotation was needed to get writes to new_id / bind
to succeed on AppArmor enabled OSes.

However, many things have changed:

* new_id should not be used anymore and it was dropped in the plugin.
* QAT initcontainer has assumed the role of HW initialization.
* vfio-pci is the preferred "dpdkDriver" and starting with QAT Gen4, it
is the only available VF driver so unbind isn't necessary.
* k8s AppArmor is "GA" since 1.30 and the annotation is deprecated.

As of now, the initcontainer will take care of binding QAT VFs to vfio-pci
so the plugin does not neeed to set AppArmor at all.

Signed-off-by: Mikko Ylinen <[email protected]>
Copy link
Contributor

@tkatila tkatila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

@tkatila tkatila merged commit f29f356 into intel:main Jan 16, 2025
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

qat: don't set device IDs in the plugin Update Apparmor configuration and related docs
3 participants