-
Notifications
You must be signed in to change notification settings - Fork 114
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
Configure IB VFs' GUIDs using a statically provided GUID pool #659
Configure IB VFs' GUIDs using a statically provided GUID pool #659
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
19e70f6
to
18d5e84
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
18d5e84
to
f6ba5ef
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
f6ba5ef
to
7cad4f9
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the PR @almaslennikov! I added few comments
7cad4f9
to
bcbd96d
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
bcbd96d
to
7492ffd
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
7492ffd
to
9cba435
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review, I need to have another look at the PR
pkg/host/internal/sriov/sriov.go
Outdated
@@ -145,14 +149,14 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork | |||
return vf | |||
} | |||
|
|||
func (s *sriov) SetVfGUID(vfAddr string, pfLink netlink.Link) error { | |||
func (s *sriov) SetVfGUID(vfAddr string, guid net.HardwareAddr, pfLink netlink.Link) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method still used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is still there. Do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still here :) i dont think its being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@almaslennikov can you address this one ? :)
9cba435
to
236e10b
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
236e10b
to
a68bf0d
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
a68bf0d
to
3823ee6
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
3823ee6
to
10dc541
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
10dc541
to
a553099
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
pkg/host/internal/sriov/sriov.go
Outdated
@@ -145,14 +149,14 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork | |||
return vf | |||
} | |||
|
|||
func (s *sriov) SetVfGUID(vfAddr string, pfLink netlink.Link) error { | |||
func (s *sriov) SetVfGUID(vfAddr string, guid net.HardwareAddr, pfLink netlink.Link) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is still there. Do we need it?
} | ||
|
||
func (i *infiniband) ConfigureVfGUID(vfAddr string, pfAddr string, vfID int, pfLink netlink.Link) error { | ||
guid := utils.GenerateRandomGUID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move GenerateRandomGUID() function to the this package (infiniband)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to infiniband package ?
a553099
to
7ba5e31
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
It looks like there is some problem with branch base. The branch contains commits which it shouldn't have. |
7332d30
to
ae28217
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once remaining comments addressed im LGTM.
Thx for the work @almaslennikov
pkg/host/internal/sriov/sriov.go
Outdated
@@ -145,14 +149,14 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork | |||
return vf | |||
} | |||
|
|||
func (s *sriov) SetVfGUID(vfAddr string, pfLink netlink.Link) error { | |||
func (s *sriov) SetVfGUID(vfAddr string, guid net.HardwareAddr, pfLink netlink.Link) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@almaslennikov can you address this one ? :)
also we should have some user documentation for this feature under doc |
ae28217
to
ca219d3
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
ca219d3
to
418c6bc
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
418c6bc
to
1e85f07
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
1e85f07
to
68e7b6b
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
68e7b6b
to
40f8039
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting close :)
40f8039
to
0978395
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! thx @almaslennikov 🎉
@SchSeba PTAL :) i believe we can merge this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work just two small comments and we can merge this one
@almaslennikov could you rebase and address comments, i think thats the last stretch b4 merging this one :) |
0978395
to
a718c3d
Compare
Signed-off-by: amaslennikov <[email protected]>
switch sriov module to use the new interface Signed-off-by: amaslennikov <[email protected]>
new package incapsulates the guid config file validation, guid pool implementation and host manipulations to set IB VF GUIDs
Signed-off-by: amaslennikov <[email protected]>
Signed-off-by: amaslennikov <[email protected]>
a718c3d
to
f199eb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Dependent on the design doc PR: #653
TBD