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

source: implement FeatureSource interface #604

Merged

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Sep 21, 2021

Convert the cpu, kernel, pci, usb, system, custom and local sources to implement the FeatureSource interface, i.e. do feature discovery and creation of feature labels separately.

Changes the related custom rules to utilize GetFeatures() method of the feature sources.

Also moves different "utils" from source/internal to the respective sources.

This PR is split from #464, to make review easier. It demonstrates usage of the new FeatureSource interface with some of the sources. Other sources will be converted in future PRs.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 21, 2021
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from f5b245c to 7ebb099 Compare September 21, 2021 13:58
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 21, 2021
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from 7ebb099 to b1073ef Compare November 2, 2021 13:40
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from b1073ef to 1facc87 Compare November 9, 2021 11:21
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 9, 2021

Rebased. Depends on #642
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2021
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from 1facc87 to 157fff1 Compare November 9, 2021 11:57
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 9, 2021

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2021
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from 157fff1 to afd6b9b Compare November 9, 2021 14:08
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 11, 2021

/assign @zvonkok @ArangoGutierrez

@zvonkok
Copy link
Contributor

zvonkok commented Nov 11, 2021

@marquiz This fails for me:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc3c369]

goroutine 1 [running]:
sigs.k8s.io/node-feature-discovery/source/kernel.(*kernelSource).GetLabels(0x157b220)
	/go/node-feature-discovery/source/kernel/kernel.go:108 +0x269
sigs.k8s.io/node-feature-discovery/pkg/nfd-client/worker.getFeatureLabels({0x7ff284548b88, 0x157b220}, {{0x0, 0x0}, 0xc0002e0c60, 0x0, 0x0, 0x15555, {0xc000276fa0, 0x1, ...}, ...})
	/go/node-feature-discovery/pkg/nfd-client/worker/nfd-worker.go:427 +0xb0
sigs.k8s.io/node-feature-discovery/pkg/nfd-client/worker.createFeatureLabels({0xc0001e73f0, 0xb, 0x203000}, {{0x0, 0x0}, 0xc0002e0c60, 0x0, 0x0, 0x15555, {0xc000276fa0, ...}, ...})
	/go/node-feature-discovery/pkg/nfd-client/worker/nfd-worker.go:408 +0x1d9
sigs.k8s.io/node-feature-discovery/pkg/nfd-client/worker.(*nfdWorker).Run(0xc0001a5ce0)
	/go/node-feature-discovery/pkg/nfd-client/worker/nfd-worker.go:187 +0xbb8
main.main()
	/go/node-feature-discovery/cmd/nfd-worker/main.go:63 +0x371

@zvonkok
Copy link
Contributor

zvonkok commented Nov 11, 2021

I think this comes from line 139 in kernel.go. When SELinux is not enabled there is no entry added.

@marquiz
Copy link
Contributor Author

marquiz commented Nov 11, 2021

I think this comes from line 139 in kernel.go. When SELinux is not enabled there is no entry added.

Argh, my latest change (#642) causes these failures (selinux is not the only possible point of panic 😬 ). I'll fix and add unit tests for this. Thanks for testing @zvonkok!

Less error prone, as no chance for a nil pointer dereference.
Separate feature discovery and creation of feature labels in the kernel
source.

Move kernelutils from source/internal back to the source/kernel package.
Change the kconfig custom rule to rely on the FeatureSource interface
(GetFeatures()) of the kernel source.

Also, add minimalist unit test.
Convert the cpu source to do feature discovery and creation of feature
labels separately.

Move cpuidutils from source/internal to the source/cpu package. Change
the cpuid custom rule to utilize GetFeatures of the cpu source.

Also, add minimalist unit test.
Separate feature discovery and creation of feature labels in the pci
source.

Move pci_utils from source/internal to the source/pci package. Change
the implementation of the PciID custom rule to utilize the FeatureSource
interface of the pci source.

Also, add minimalist unit test.
Separate feature discovery and creation of feature labels in the usb
source.

Move usb_utils from source/internal to the source/usb package. Change
the implementation of the UsbID custom rule to utilize the FeatureSource
interface of the usb source.

Also, add minimalist unit test.
Move the functionality responsible for detection of loeaded kernel
modules from source/custom over to the source/kernel package. Add a new
"loadedmodule" raw feature to the kernel source to store this
information.

Change loadedKmod custom rule to utilize kernel source.
Separate feature discovery and creation of feature labels in the system
source.

Also, change the implementation of the nodeName custom rule to utilize
the FeatureSource interface of the system source.

Also, add minimalist unit test.
Separate feature discovery (i.e. running hooks and reading feature
files) and creation of feature labels in the local source.

Also, add minimalist unit test.
@marquiz marquiz force-pushed the devel/feature-source-conversion branch from afd6b9b to a91f332 Compare November 11, 2021 16:34
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 11, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 11, 2021

OK, this should be fixed, now. Fixed by reverting part of my earlier internal api change.

I also added simple unit tests to the feature sources to avoid this.

@ArangoGutierrez
Copy link
Contributor

e2e-test works on Minikube 1.22

Ran 2 of 230 Specs in 24.488 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 228 Skipped
--- PASS: TestE2E (24.51s)
PASS
ok      sigs.k8s.io/node-feature-discovery/test/e2e     24.543s

test

go test ./cmd/... ./pkg/... ./source/...                                                                                                                                                                                                      
?       sigs.k8s.io/node-feature-discovery/cmd/nfd-master       [no test files]                                                                                                                                                               
ok      sigs.k8s.io/node-feature-discovery/cmd/nfd-topology-updater     (cached)                          
ok      sigs.k8s.io/node-feature-discovery/cmd/nfd-worker       0.012s                                                 
?       sigs.k8s.io/node-feature-discovery/pkg/api/feature      [no test files]                     
?       sigs.k8s.io/node-feature-discovery/pkg/apihelper        [no test files]                                        
?       sigs.k8s.io/node-feature-discovery/pkg/cpuid    [no test files]        
ok      sigs.k8s.io/node-feature-discovery/pkg/kubeconf (cached)                                                       
?       sigs.k8s.io/node-feature-discovery/pkg/labeler  [no test files]                                                
?       sigs.k8s.io/node-feature-discovery/pkg/nfd-client       [no test files]                                        
ok      sigs.k8s.io/node-feature-discovery/pkg/nfd-client/topology-updater      (cached)                                                                                                                                                      
ok      sigs.k8s.io/node-feature-discovery/pkg/nfd-client/worker        3.093s                                         
ok      sigs.k8s.io/node-feature-discovery/pkg/nfd-master       (cached)                                               
?       sigs.k8s.io/node-feature-discovery/pkg/podres   [no test files]                                                
ok      sigs.k8s.io/node-feature-discovery/pkg/resourcemonitor  (cached)                                          
?       sigs.k8s.io/node-feature-discovery/pkg/topologypolicy   [no test files]                                        
?       sigs.k8s.io/node-feature-discovery/pkg/topologyupdater  [no test files]                                        
ok      sigs.k8s.io/node-feature-discovery/pkg/utils    (cached)                                                       
?       sigs.k8s.io/node-feature-discovery/pkg/version  [no test files]                                                                                                                                                                       
ok      sigs.k8s.io/node-feature-discovery/source       0.033s                                                         
ok      sigs.k8s.io/node-feature-discovery/source/cpu   0.034s                                                         
?       sigs.k8s.io/node-feature-discovery/source/custom        [no test files]                                        
?       sigs.k8s.io/node-feature-discovery/source/custom/rules  [no test files]                                                                                                                                                               
?       sigs.k8s.io/node-feature-discovery/source/fake  [no test files]                                                                                                                                                                       ?       sigs.k8s.io/node-feature-discovery/source/iommu [no test files]                                                
ok      sigs.k8s.io/node-feature-discovery/source/kernel        0.038s                                                 
ok      sigs.k8s.io/node-feature-discovery/source/local 0.030s                                                         
?       sigs.k8s.io/node-feature-discovery/source/memory        [no test files]                                        
?       sigs.k8s.io/node-feature-discovery/source/network       [no test files]                         
ok      sigs.k8s.io/node-feature-discovery/source/pci   0.007s                                          
?       sigs.k8s.io/node-feature-discovery/source/storage       [no test files]                                        
ok      sigs.k8s.io/node-feature-discovery/source/system        0.004s                                                 
ok      sigs.k8s.io/node-feature-discovery/source/usb   0.004s   

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, 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:

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 merged commit 5299ca2 into kubernetes-sigs:master Nov 11, 2021
@marquiz marquiz deleted the devel/feature-source-conversion branch November 11, 2021 17:49
@marquiz marquiz mentioned this pull request Dec 22, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants