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

Allow to filter net devices by Infiniband Partition Key #517

Merged
merged 6 commits into from
Feb 18, 2024

Conversation

almaslennikov
Copy link
Contributor

@almaslennikov almaslennikov commented Dec 14, 2023

This PR introduced a new kind of device selector based on Infiniband Partition Key.
The PKey retrieval functionality is taken from sriovnet. This PR will be finalized with updated sriovnet dependency when the (related PR there) is merged

@coveralls
Copy link
Collaborator

coveralls commented Dec 14, 2023

Pull Request Test Coverage Report for Build 7914659039

Details

  • -26 of 46 (43.48%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 75.165%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/utils/sriovnet_provider.go 0 3 0.0%
pkg/netdevice/pciNetDevice.go 3 11 27.27%
pkg/utils/utils.go 0 15 0.0%
Totals Coverage Status
Change from base Build 7738698138: -0.5%
Covered Lines: 2052
Relevant Lines: 2730

💛 - Coveralls

@almaslennikov almaslennikov marked this pull request as ready for review January 8, 2024 11:26
Copy link
Collaborator

@Eoghan1232 Eoghan1232 left a comment

Choose a reason for hiding this comment

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

@almaslennikov Thanks for the contribution!
this looks good to me, could you fix the lint issue as well? just remove the commented out line.

@almaslennikov almaslennikov force-pushed the pkey-selector branch 2 times, most recently from bbaac0f to 073df9b Compare January 29, 2024 07:50
@almaslennikov
Copy link
Contributor Author

The new sriovnet dependency requires upgrading to go1.21. I've created a separate PR to update the toolchain: #527

@adrianchiris
Copy link
Contributor

adrianchiris commented Feb 1, 2024

@almaslennikov can you sort out lint issue?

@almaslennikov almaslennikov force-pushed the pkey-selector branch 2 times, most recently from 15ce663 to a8f45fb Compare February 1, 2024 09:13
@almaslennikov
Copy link
Contributor Author

@adrianchiris @Eoghan1232 the linter issues are fixed

Copy link
Collaborator

@Eoghan1232 Eoghan1232 left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for fixing linting issue

@almaslennikov
Copy link
Contributor Author

/retest

@almaslennikov
Copy link
Contributor Author

/retest-kubernetes-ci

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

overall this looks good. added a few small comments.

pkg/utils/sriovnet_provider.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
This function wraps sriovnet.GetDefaultPKeyFromPci and returns
the default IB Partition Key for an Infiniband device

Signed-off-by: amaslennikov <[email protected]>
Add a new interface function + implement it in pciNetDevice struct

Signed-off-by: amaslennikov <[email protected]>
Signed-off-by: amaslennikov <[email protected]>
Signed-off-by: amaslennikov <[email protected]>
@almaslennikov
Copy link
Contributor Author

@adrianchiris Thank you for the review! Addressed your comments

Signed-off-by: amaslennikov <[email protected]>
Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

LGTM ! . Thx @almaslennikov :)

@adrianchiris
Copy link
Contributor

@Eoghan1232 could you take a quick look at the lastest iteration ? if all is ok id like to merge this one

Copy link
Collaborator

@Eoghan1232 Eoghan1232 left a comment

Choose a reason for hiding this comment

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

latest iteration lgtm.

@Eoghan1232
Copy link
Collaborator

@adrianchiris are we waiting for all CI to pass or can we merge this?

@adrianchiris
Copy link
Contributor

we can merge. we have issues with CI that need to be sorted internally.

@adrianchiris adrianchiris merged commit 1a99d09 into k8snetworkplumbingwg:master Feb 18, 2024
9 of 10 checks passed
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.

4 participants