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

Add acpiIndex selector #488

Merged

Conversation

ian-howell
Copy link
Contributor

No description provided.

@ian-howell ian-howell mentioned this pull request Jun 2, 2023
@coveralls
Copy link
Collaborator

coveralls commented Jun 6, 2023

Pull Request Test Coverage Report for Build 5159544610

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 29 of 45 (64.44%) changed or added relevant lines in 5 files are covered.
  • 28 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.6%) to 77.747%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/netdevice/netDeviceProvider.go 7 8 87.5%
pkg/factory/factory.go 8 10 80.0%
pkg/utils/utils.go 0 13 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/devices/host.go 6 83.72%
pkg/factory/factory.go 9 88.73%
pkg/netdevice/netDeviceProvider.go 13 84.07%
Totals Coverage Status
Change from base Build 5055671149: -0.6%
Covered Lines: 1939
Relevant Lines: 2494

💛 - Coveralls

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

this looks great! nice work!

@ian-howell ian-howell force-pushed the dev/acpi-index-selector branch from 2facafd to 9f71e85 Compare June 26, 2023 16:36
@ian-howell
Copy link
Contributor Author

Fixed the merge conflicts.

@Eoghan1232 @adrianchiris @SchSeba Gentle nudge to request reviews when you find time. Thanks in advance!

@ian-howell ian-howell force-pushed the dev/acpi-index-selector branch from a27864d to e171535 Compare June 27, 2023 17:39
@@ -382,6 +382,24 @@ func GetDriverName(pciAddr string) (string, error) {
return filepath.Base(driverInfo), nil
}

// GetAcpiIndex returns the ACPI index attached to a pci device from its pci address
func GetAcpiIndex(pciAddr string) (string, error) {
acpiIndexLink := filepath.Join(sysBusPci, pciAddr, "acpi_index")
Copy link
Contributor

Choose a reason for hiding this comment

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

some questions on acpi index:

when will i have an acpi_index ?
do i need to boot using UEFI to have this index ?
do i need some special kernel cmdline flag to expose it ?

checked on baremetal machine an NVIDIA NIC, it did not have an acpi index so probably something is not enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I checked on my system with an Intel NIC, it did not have an acpi index, so I am guessing something in bios must be enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain what actually causes a machine to get an acpi_index. As SchSheba pointed out, this is inside of a libvirt VM, (or more specifically for my case, a kubevirt VM using this field).

pkg/types/types.go Outdated Show resolved Hide resolved
pkg/types/types.go Outdated Show resolved Hide resolved
pkg/devices/host.go Outdated Show resolved Hide resolved
pkg/factory/factory.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

/LGTM

@ian-howell
Copy link
Contributor Author

Is there anything left to do prior to merging this?

@Eoghan1232
Copy link
Collaborator

@adrianchiris are you okay for this to be merged?

@adrianchiris
Copy link
Contributor

lemme have a quick look

@adrianchiris adrianchiris merged commit 81a0e6a into k8snetworkplumbingwg:master Oct 5, 2023
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.

6 participants