UDN: Prerequisite: Make OVN topology network aware. #4478
Conversation
dceara
added
the
feature/user-defined-network-segmentation
github-actions
bot
added
feature/egress-ip
dceara
force-pushed
the
net-seg-make-topo-net-aware
branch
2 times, most recently
from
9164d25
to
1c1c8d5
Compare
|
@tssurya This at least passes UTs. Marking it as ready for review in the hope that it also passes e2e. |
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the port name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
And use the GetNetworkScopedK8sMgmtIntfName() wrapper instead of buidling the port name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the port name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
It makes it easier to use generic network aware wrappers in the future. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of hardcoding the cluster router name. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the name manually. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
…ter name. This allows us to use it from multiple networks. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the name manually. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the name manually. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the name manually. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the GW router name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
…ame. This allows us to use it from multiple networks. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the GW router name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the GW router name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the router name ad-hoc. TODO: this still isn't network aware! Signed-off-by: Dumitru Ceara <dceara@redhat.com>
…omNode(). TODO: there are still a few places, e.g., apbroute, where we use GetGatewayRouterFromNode() directly. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of hardcoding the join switch name. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Instead of building the node switch name ad-hoc. Signed-off-by: Dumitru Ceara <dceara@redhat.com>
dceara
force-pushed
the
net-seg-make-topo-net-aware
branch
from
1c1c8d5
to
2b29179
Compare
| GetNetworkScopedK8sMgmtIntfName(nodeName string) string | ||
| GetNetworkScopedClusterRouterName() string | ||
| GetNetworkScopedGWRouterName(nodeName string) string | ||
| GetNetworkScopedSwitchName(nodeName string) string | ||
| GetNetworkScopedJoinSwitchName() string | ||
| GetNetworkScopedExtSwitchName(nodeName string) string |
There was a problem hiding this comment.
nit: Since these are really under under NetInfo they are suppose to be NetworkScoped maybe we can remove that part although I see they are used as oc.GetNetworkScopedClusterRouteName.
There was a problem hiding this comment.
As discussed offline, it's probably more explicit to keep them as is. Otherwise:
oc.GetNetworkScopedClusterRouteName()
would change to:
oc.GetClusterRouteName()
@tssurya Wdyt?
There was a problem hiding this comment.
I like the NetworkScoped part
There was a problem hiding this comment.
Not at desk(will come back to this again), but in future PRs do we plan to use this from everywhere in BNC? Where its internally going to be defaultnetwork(empty) and other networks? Ny preference depends on that..
Like do we plan to keep getrouterName etc in DNC as is? And this is extra on top?
go-controller/pkg/kubevirt/router.go
Outdated
| @@ -94,7 +94,7 @@ func EnsureLocalZonePodAddressesToNodeRoute(watchFactory *factory.WatchFactory, | |||
| if config.OVNKubernetesFeature.EnableInterconnect { | |||
| // NOTE: EIP & ESVC use same route and if this is already present thanks to those features, | |||
| // this will be a no-op | |||
| if err := libovsdbutil.CreateDefaultRouteToExternal(nbClient, pod.Spec.NodeName); err != nil { | |||
| if err := libovsdbutil.CreateDefaultRouteToExternal(nbClient, types.OVNClusterRouter, pod.Spec.NodeName); err != nil { | |||
There was a problem hiding this comment.
this seems a little awkard to me. The commit talks about making it for multiple networks, so I would have expected refactoring this to use oc.GetNetworkScopedClusterRouter or whatever the function is. Also the EnsureLocalZonePodAddressesToNodeRoute function passes in the nadName. Maybe this is just something to clean up later.
There was a problem hiding this comment.
Indeed, the kubevirt part needs further cleanup. I kind of gave up half way there. Let's do that as a follow up, thanks for your review!
| @@ -94,7 +94,7 @@ func EnsureLocalZonePodAddressesToNodeRoute(watchFactory *factory.WatchFactory, | |||
| if config.OVNKubernetesFeature.EnableInterconnect { | |||
| // NOTE: EIP & ESVC use same route and if this is already present thanks to those features, | |||
| // this will be a no-op | |||
| if err := libovsdbutil.CreateDefaultRouteToExternal(nbClient, types.OVNClusterRouter, pod.Spec.NodeName); err != nil { | |||
| if err := libovsdbutil.CreateDefaultRouteToExternal(nbClient, types.OVNClusterRouter, types.GWRouterPrefix+pod.Spec.NodeName); err != nil { | |||
There was a problem hiding this comment.
another instance of my previous comment in the same file. I think we should either make functions in kubevirt receivers of the controller, or pass the controller in here and use the correct function rather than fixing this behavior of string concat in some places by using functions, but here we still prepend the prefix.
|
odd APB Route flaked: https://github.com/ovn-org/ovn-kubernetes/actions/runs/9710194825/job/26811460206?pr=4478 Looks like its being tracked here: #4432 |
What this PR does and why is it needed
This PR is a prerequisite for adding support for L2/L3 user defined network support.
It adds helpers to avoid hardcoding OVN topology names (cluster router, node switches, managament ports, external switches, GW routers).
It changes (most of) the code base to use the new wrappers. There are still some places where names are built in an ad-hoc manner, e.g., some parts of the hybrid overlay implementation and the kubevirt package. The latter doesn't use a network controller derived from BaseNetworkController so making it network aware is left as a follow up task.
Which issue(s) this PR fixes
Fixes #
Special notes for reviewers
There are quite a few commits in this PR but the rationale is to make the transition to the new helpers in chunks, per functionality to allow for a simpler review process.
How to verify it
This doesn't change functionality so existing unit and e2e tests must pass as-is.
Details to documentation updates
No doc changes needed.
Description for the changelog
Don't hardcode OVN topology names.
Does this PR introduce a user-facing change?