search

intel / intel-device-plugins-for-kubernetes

Collection of Intel device plugins for Kubernetes
Apache License 2.0
48 stars 205 forks source link

qat, initcontainer: add enablement of auto_reset #1852

Closed hj-johannes-lee closed 1 month ago

hj-johannes-lee commented 1 month ago

For some reason, the previous pr #1838 is closed. I make another pr.

mythi commented 1 month ago

our target has been to eventually align with qatlib's qat_init.sh and this is taking us further from that.

hj-johannes-lee commented 1 month ago

@mythi Thanks!

our target has been to eventually align with qatlib's qat_init.sh and this is taking us further from that.

Umm... would it be better to make totally independent script? Or another function in qat-init.sh? To be honest, I do not see any reason why we need to keep it similar to that. I ofc checked the qat-init.sh of qatlib before starting the work, but there is no part about "setting auto_reset". If we make anything about auto_reset in qat-init.sh, then it is already far from the qatlib's init script.

@tkatila Thank you for your comments

Change check_config to return true/false depending on the check result. Drop is_found

That was my first attempt, but unfortunately return is not implemented in toybox (http://lists.landley.net/pipermail/toybox-landley.net/2023-June/029623.html).

   Would remove some of the intermediate variables like AUTORESET_ENABLED_FOUND

Write the sriov and auto_reset activations like: check_config "ServicesEnabled" "$SERVICES_LIST" && sysfs_config && enable_sriov check_config "AutoresetEnabled" "$AUTORESET_OPTIONS_LIST" && enable_auto_reset Separate "options check" into its own function

Thanks. Let me consider if we can "go further" from the qatlib's script. I need to wait what Mikko says about the the script...

mythi commented 1 month ago

I do not see any reason why we need to keep it similar to that.

the reason is we want to follow their work as much as possible and provide the exact same experience for users with and without k8s.

hj-johannes-lee commented 1 month ago

the reason is we want to follow their work as much as possible and provide the exact same experience for users with and without k8s.

So,, what do you think it's better? Just.. another function (but will be basically copy and paste of check_config) in qat-init.sh or just different script in the image?

mythi commented 1 month ago

it could be a simple add-on, e.g., using . "$(dirname "$0")/qat-autoreset.sh". my preference is to not touch the existing code/functions too much

hj-johannes-lee commented 1 month ago

Then, I think it is even better to not make qat-init.sh be related to qat-autoreset.sh I just changed the dockerfile.

hj-johannes-lee commented 1 month ago

Idk if we would want to add more things in the future for the initcontainer part. If it happens, we may consider changing the name of qat-autoreset.sh to something else.

hj-johannes-lee commented 1 month ago

@mythi @tkatila Hi, could you review this?

mythi commented 1 month ago

@mythi @tkatila Hi, could you review this?

Looks OK to me but the text here needs some fixes: "qat, initcontaniner: imporve the logic of config check".

tkatila commented 1 month ago

Is the origin this script: https://github.com/intel/qatlib/blob/main/quickassist/utilities/service/qat_init.sh.in ?

hj-johannes-lee commented 1 month ago

Is the origin this script: https://github.com/intel/qatlib/blob/main/quickassist/utilities/service/qat_init.sh.in ?

@tkatila Yes!

hj-johannes-lee commented 1 month ago

@tkatila Yeah... I have not liked the script either since I made a qat-initcontainer..! :sweat_smile: :sweat_smile:

hj-johannes-lee commented 1 month ago

@mythi Could you merge this if it looks good to you?