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.

Sign up for GitHub

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

Jump to bottom

Adding support for firmware loading #596

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Adding support for firmware loading #596

merged 1 commit into from
Oct 10, 2023

Conversation

yevgeny-shnaidman
Copy link
Contributor

  1. mapping of the host firmware directory into the load/unload pod only
    in case firmware for the Module is defined
  2. during Loading operation, worker pod is copying all firmware files and
    directories from image onto the mapped host directory
  3. during Unloading operation, worker pod is deleting all firmware
    files, but not directories ( since those can be shared with other
    modules' firmware files
  4. unit tests

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yevgeny-shnaidman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested a review from mresvanis October 8, 2023 00:39
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 8, 2023
@netlify
Copy link

netlify bot commented Oct 8, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name Link
🔨 Latest commit adecbad
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/65240f2878345400084da0e4
😎 Deploy Preview https://deploy-preview-596--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 8, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (20fe966) 79.27% compared to head (455cc54) 78.73%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
- Coverage   79.27%   78.73%   -0.55%     
==========================================
  Files          49       49              
  Lines        4849     4937      +88     
==========================================
+ Hits         3844     3887      +43     
- Misses        829      866      +37     
- Partials      176      184       +8
Files Coverage Δ
cmd/worker/funcs_kmod.go 83.33% <100.00%> (+2.08%) ⬆️
cmd/worker/main.go 0.00% <0.00%> (ø)
internal/controllers/nmc_reconciler.go 77.05% <70.58%> (-0.40%) ⬇️
internal/worker/worker.go 79.81% <46.66%> (-12.78%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

qbarrand
qbarrand reviewed Oct 9, 2023
View reviewed changes
cmd/worker/funcs_kmod_test.go Outdated Show resolved Hide resolved
cmd/worker/main.go Outdated Show resolved Hide resolved
cmd/worker/main.go Outdated Show resolved Hide resolved
config/manager/kustomization.yaml Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated Show resolved Hide resolved
internal/worker/worker.go Outdated
if cfg.Modprobe.FirmwarePath != "" {
baseImageDir := pr.fsDir + "/" + cfg.Modprobe.FirmwarePath
w.logger.Info("preparing fimrware for loading", "image directory", baseImageDir, "host mount directory", firmwareMountPath)
if err = cp.Copy(baseImageDir, firmwareMountPath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What this function does in case baseImageDir exists is not very clear:

	// Merge preserves or overwrites existing files under the dir (default behavior).

https://pkg.go.dev/github.com/otiai10/copy#DirExistsAction
When does it replace and when does it overwrite?
Also, WDYT about using the OnError option to log copy errors that may occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You probably mean in case there are already directories/files exist at destination, right? I have looked at the copy package code and run a few tests, and i think we are ok: if directory already exists, then new files will be added, files with the same name will be overwritten, files present only at dest will be kept as they are.
Also, added onError function

@yevgeny-shnaidman
Adding support for firmware loading
adecbad 
1) mapping of the host firmware directory into the load/unload pod only
   in case firmware for the Module is defined
2) during Loading operation, worker pod is copying all firmware files and
   directories from image onto the mapped host directory
3) during Unloading operation, worker pod is deleting all firmware
   files, but not directories ( since those can be shared with other
   modules' firmware files
4) unit tests
@yevgeny-shnaidman yevgeny-shnaidman force-pushed the yevgeny/firmware-support branch from 455cc54 to adecbad Compare October 9, 2023 14:33
@qbarrand
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit c50eeff into kubernetes-sigs:main Oct 10, 2023
12 checks passed
@qbarrand qbarrand linked an issue Oct 10, 2023 that may be closed by this pull request
Add support for firmware files #492
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qbarrand qbarrand qbarrand left review comments

@mresvanis mresvanis Awaiting requested review from mresvanis

Assignees

@qbarrand qbarrand

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for firmware files
4 participants
@yevgeny-shnaidman @k8s-ci-robot @codecov-commenter @qbarrand