-
Notifications
You must be signed in to change notification settings - Fork 109
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
[software-bridges 3/x] Add bridge package #691
[software-bridges 3/x] Add bridge package #691
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9958277243Details
💛 - Coveralls |
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.
I left some comments in general the code looks good.
just one comment when we are not running in systemdmode the configuration will be done after a chroot? if not we need to update the ovs socket file to support the /host mount inside the container
CreateOVSBridge(ctx context.Context, conf *sriovnetworkv1.OVSConfigExt) error | ||
// GetOVSBridges returns configuration for all managed bridges | ||
GetOVSBridges(ctx context.Context) ([]sriovnetworkv1.OVSConfigExt, error) | ||
// RemoveOVSBridge removes managed OVS bridge by name |
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.
do we also want/need getOVSBridge(name string)?
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 not required by the implementation at the moment. I don't want to add this function just for API fullness
pkg/host/internal/bridge/ovs/ovs.go
Outdated
// if OVS bridge exist with different config it will be removed and re-created | ||
func (o *ovs) CreateOVSBridge(ctx context.Context, conf *sriovnetworkv1.OVSConfigExt) error { | ||
ctx, cFunc := setDefaultTimeout(ctx) | ||
defer cFunc() |
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.
nit: can you change cFunc to just cancel please ;)
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.
done
pkg/host/internal/bridge/ovs/ovs.go
Outdated
return nil | ||
} | ||
|
||
// RemoveInterfaceFromOVSBridge interface from the managed OVS bridge |
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.
please add: removes interface from the managed OVS bridge
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.
done
pkg/host/internal/bridge/ovs/ovs.go
Outdated
} | ||
|
||
// RemoveInterfaceFromOVSBridge interface from the managed OVS bridge | ||
func (o *ovs) RemoveInterfaceFromOVSBridge(ctx context.Context, ifaceAddr string) 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.
ifaceAddr -> pciAddress it looks like it's ip address
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.
done
pkg/host/internal/bridge/ovs/ovs.go
Outdated
func (o *ovs) RemoveInterfaceFromOVSBridge(ctx context.Context, ifaceAddr string) error { | ||
ctx, cFunc := setDefaultTimeout(ctx) | ||
defer cFunc() | ||
funcLog := log.Log.WithValues("ifaceAddr", ifaceAddr) |
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.
same here ifaceAddr -> pciAddress
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.
done
pkg/host/internal/bridge/ovs/ovs.go
Outdated
} | ||
|
||
// delete bridge by the name | ||
func (o *ovs) deleteBridge(ctx context.Context, dbClient client.Client, brName string) 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.
deleteBridgeByName
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.
done
pkg/host/internal/bridge/ovs/ovs.go
Outdated
} | ||
|
||
// delete interface by the name | ||
func (o *ovs) deleteInterface(ctx context.Context, dbClient client.Client, ifaceName string) 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.
deleteInterfaceByName
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.
done
partial review for now. added a few comments :) |
cdec0b4
to
095dcb4
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@adrianchiris @SchSeba thanks for your reviews! I addressed your comments. PTAL |
095dcb4
to
51697bf
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
PR reabased. Currently it has no dependencies from other PRs |
51697bf
to
e4b4f24
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 !!
e4b4f24
to
6d0b303
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
The PR rebased. @SchSeba I think all comments are addressed. Can you take another look? Thanks |
@SchSeba can we merge this one ? |
6d0b303
to
47724b7
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.
Only two minor comments on my side. Otherwise LGTM
As soon as we have a full working feature, I want to check if and how config-daemon logs are affected by bridge management. We're already suffering there and I really need them to not grow much.
The package contains logic to work with managed OVS bridges. // DiscoverBridges returns information about managed bridges on the host DiscoverBridges() (sriovnetworkv1.Bridges, error) // ConfigureBridge configure managed bridges for the host ConfigureBridges(bridgesSpec sriovnetworkv1.Bridges, bridgesStatus sriovnetworkv1.Bridges) error // DetachInterfaceFromManagedBridge detach interface from a managed bridge, // this step is required before applying some configurations to PF, e.g. changing of eSwitch mode. // The function detach interface from managed bridges only. DetachInterfaceFromManagedBridge(pciAddr string) error Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>
47724b7
to
a801939
Compare
@zeeke Thanks for the review. I addressed your comments. Please, take another look |
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.
Great work!
The package contains logic to work with managed OVS bridges.
Context is Software bridge management feature