add swap support in nfd

[kannon92]

Fixes #1584

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	187f65f
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/65d371e507181500082269a4
😎 Deploy Preview	https://deploy-preview-1585--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[kannon92]

/hold

Still need the deployment changes.

[marquiz]

Thanks @kannon92 for the patch. Some quick notes below

[AhmedThresh]

@kannon92 I think that you should mention the swap feature in the docs/usage/features.md.

[AhmedThresh]

@kannon92 Thanks for your efforts!

[kannon92]

@marquiz @AhmedThresh I am new to this project so I don't know if I should have e2e tests or unit tests covering these changes.

[AhmedThresh]

@marquiz @AhmedThresh I am new to this project so I don't know if I should have e2e tests or unit tests covering these changes.

These kinds of features generally are not included in the e2e tests AFAIK. However, I see that we can implement a unit test for the getNumberOfLinesFromFile function. wdyt?

[marquiz]

I think that you should mention the swap feature in the docs/usage/features.md.

Good point. In addition, document it in the table of "available features" in docs/usage/customization-guide.md, too.

Also, a nit about the commit message title (ref https://www.kubernetes.dev/docs/guide/pull-requests/#commit-message-guidelines): start with a capital letter, e.g. Add swap support, or alternatively provide context like source/network: detect swap or smth

[marquiz]

I am new to this project so I don't know if I should have e2e tests or unit tests covering these changes.

@kannon92 very good question. We have been lacking on unit testing of the feature detection 🙃 It'd be good to add a simple unit test as @AhmedThresh suggested.

I've long had in my backlog to add more comprehensive unit tests for each source/feature that rely on scanning host fs. Have a fake rootfs and run feature discovery against that. But you can skip this (unless you want to work on it) as there is not infrastructure or prior art, yet (I have a rough poc for the pci source, maybe I clean that up and submit as a pr...)

[kannon92]

I am new to this project so I don't know if I should have e2e tests or unit tests covering these changes.

@kannon92 very good question. We have been lacking on unit testing of the feature detection 🙃 It'd be good to add a simple unit test as @AhmedThresh suggested.

I've long had in my backlog to add more comprehensive unit tests for each source/feature that rely on scanning host fs. Have a fake rootfs and run feature discovery against that. But you can skip this (unless you want to work on it) as there is not infrastructure or prior art, yet (I have a rough poc for the pci source, maybe I clean that up and submit as a pr...)

So in some gigs, I have been told that any unit test involving file io is not really a true unit..

Some options for me could be to add some mock data for /proc/swaps and confirm the two cases (header versus swap partition).

Or we could go with gomocks?

[marquiz]

So in some gigs, I have been told that any unit test involving file io is not really a true unit..

Perhaps so when looking at textbook definitions. Maybe we can then call it integration test or something else. But in our case where the functionality is very much about reading data from the filesystem using file io as the "mock" is pretty handy and gives good coverage, IMO.

Some more complicated parsing deserves "classical" unit testing for sure (I took a stab at that in #1588, something that had been lying in my unfinished devel backlog for a while).

Or we could go with gomocks?

TBH, I don't see much sense in wiring in some mocked filesystem interface over just having mocked data in fs. I'll leave this for you to consider, do whatever makes sense.

FYI, I cleaned up my pci testing work and submitted #1589. It uses mocked sysfs on disk. Not classical unit test but I think makes sense there

[kannon92]

updated with your comments.

[kannon92]

/hold cancel

[marquiz]

Thanks @kannon92 for the work. Looks good to me now 👍

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedThresh, kannon92, marquiz

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:

• OWNERS [marquiz]

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

[marquiz]

/assign @ArangoGutierrez

[kannon92]

ping @ArangoGutierrez

Swap KEP was merged into 1.30 k8s so it'd be nice to see this progress.

[marquiz]

ping @ArangoGutierrez

[marquiz]

/assign @TessaIO

[TessaIO]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 5b42c1d0224800dab6f3e52a52711348c94cb8cf

[codecov]

Codecov Report

Attention: Patch coverage is 26.08696% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 31.67%. Comparing base (e0b8a52) to head (187f65f).
Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
+ Coverage   29.27%   31.67%   +2.40%     
==========================================
  Files         101      103       +2     
  Lines        9340     9888     +548     
==========================================
+ Hits         2734     3132     +398     
- Misses       6336     6476     +140     
- Partials      270      280      +10     

Files	Coverage Δ
pkg/utils/hostpath/hostpath.go	100.00% <ø> (ø)
source/memory/memory.go	19.80% <26.08%> (+1.85%)	⬆️

... and 16 files with indirect coverage changes