Skip to content
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

operator: fix setting QAT provisioning config volumeMount #1431

Merged
merged 1 commit into from
May 25, 2023

Conversation

mythi
Copy link
Contributor

@mythi mythi commented May 23, 2023

setInitContainer() adds "init-sriov-numvfs" to initContainers but uses initcontainerName constant to search where to add the QAT configMap volumeMount. Fix by moving all code to use the const.

It was also noticed in the controller logs that setting Pod Volumes is not idempotent but broken DaemonSet gets created:

""intel-device-plugins-manager: Reconciler error "err="DaemonSet.apps "intel-qat-plugin" is invalid: spec.template.spec.volumes[6].name: Duplicate value: "qat-config"" controller="qatdeviceplugin" controllerGroup="deviceplugin.intel.com"

Finally, change 'qat-config' to 'intel-qat-config-volume' to better describe that it's a volume.

@mythi
Copy link
Contributor Author

mythi commented May 23, 2023

@hj-johannes-lee I need your help testing this. I did an initial pass but pls check all the possible user flows on your side too

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2023

Codecov Report

Merging #1431 (344f94b) into main (a5c6243) will decrease coverage by 0.22%.
The diff coverage is 0.00%.

❗ Current head 344f94b differs from pull request most recent head 52d3d4a. Consider uploading reports for the commit 52d3d4a to get more accurate results

@@            Coverage Diff             @@
##             main    #1431      +/-   ##
==========================================
- Coverage   51.07%   50.86%   -0.22%     
==========================================
  Files          44       44              
  Lines        5010     5013       +3     
==========================================
- Hits         2559     2550       -9     
- Misses       2305     2317      +12     
  Partials      146      146              
Impacted Files Coverage Δ
pkg/controllers/qat/controller.go 9.72% <0.00%> (-5.11%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hj-johannes-lee
Copy link
Contributor

Sure, let me do!

@hj-johannes-lee
Copy link
Contributor

hj-johannes-lee commented May 24, 2023

built an operator from your branch and made it pulled Always.
Testing with the image, I tested

  1. creating without putting any provisioningConfig value && updating yaml to have it and set as dc.
  2. creating with provisioningConfig (dc) && updating configMap and yaml (cy).
  3. cy first and updating to dc
    Is there any possible flow related to this pr?

@mythi
Copy link
Contributor Author

mythi commented May 24, 2023

  1. Pre-provision manually and deploy without initImage. Update it (kubectl edit) to add it and then remove it again

@mythi
Copy link
Contributor Author

mythi commented May 24, 2023

4. Pre-provision manually and deploy without initImage. Update it (kubectl edit) to add it and then remove it again

and that no Reconcile errors show up in controller's logs

@hj-johannes-lee
Copy link
Contributor

(I am now trying to debug found errors!)

setInitContainer() adds "init-sriov-numvfs" to initContainers
but uses initcontainerName constant to search where to add
the QAT configMap volumeMount. Fix by moving all code to use
the const.

It was also noticed in the controller logs that setting Pod
Volumes is not idempotent but broken DaemonSet gets created:

""intel-device-plugins-manager: Reconciler error "err="DaemonSet.apps
\"intel-qat-plugin\" is invalid: spec.template.spec.volumes[6].name:
Duplicate value: \"qat-config\"" controller="qatdeviceplugin"
controllerGroup="deviceplugin.intel.com"

Finally, change 'qat-config' to 'intel-qat-config-volume' to
better describe that it's a volume.

Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
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.

existing errors are not related to this pr.
LGTM! :)

@tkatila tkatila merged commit 819d302 into intel:main May 25, 2023
70 of 71 checks passed
@tkatila tkatila mentioned this pull request Aug 8, 2023
19 tasks
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.

None yet

4 participants