UDN: L3: Use nodesubnet annotations for L3; not clustersubnet from NAD

[tssurya]

For UDN L3, it seems the local LRSR and LRPs are not getting created correctly.

SGW:
See a snippet of default network routes:

sh-5.2# ovn-nbctl lr-route-list ovn_cluster_router                                                                                                                          
IPv4 Routes
Route Table <main>:
               100.64.0.2                100.88.0.2 dst-ip
               100.64.0.3                100.64.0.3 dst-ip
               100.64.0.4                100.88.0.4 dst-ip
            10.244.0.0/24                100.88.0.2 dst-ip --> remote pod subnet re-route to other node's transit port
            10.244.2.0/24                100.88.0.4 dst-ip --> remote pod subnet re-route to other node's transit port
            10.244.1.0/24                100.64.0.3 src-ip --> local pod subnet re-route to GR of this node
            10.244.0.0/16                100.64.0.3 src-ip ----> USED FOR EGRESSIPs so that packets between egressIP pods don't get dropped.

versus:
UDN routes:

sh-5.2# ovn-nbctl lr-route-list xfrkk_tenant.red_ovn_cluster_router
IPv4 Routes
Route Table <main>:
               100.65.0.2                100.88.0.2 dst-ip
               100.65.0.3                100.65.0.3 dst-ip
               100.65.0.4                100.88.0.4 dst-ip
            10.128.0.0/24                100.88.0.2 dst-ip --> remote pod subnet re-route to other node's transit port
            10.128.2.0/24                100.88.0.4 dst-ip --> remote pod subnet re-route to other node's transit port
            10.128.0.0/16                100.65.0.3 src-ip --> supposed to be local pod subnet re-route to GR of this node but looks like we are using the entire cluster subnet which is causing issues. ACTUAL ROUTE SHOULD BE 10.128.1.0/24 which is the local pod subnet here for this node

we are having an incorrect one... the local pod subnet
and the /16 one is supposed to be added from EIP code here: CreateDefaultRouteToExternal that will come only when EIP support comes on UDN.

Also seems like:

      1004 inport == "rtos-xfrkk_tenant.red_ovn-worker" && ip4.dst == 172.18.0.3 /* xfrkk_tenant.red_ovn-worker */         reroute                10.128.0.2                 

is pulling info from cluster-subnet and not node-subnet for L3 which leads to issues like:

2024-09-12T18:41:19.353Z|00017|northd|WARN|No path for routing policy priority 1004; next hop 10.128.0.2                                                                     

so here the managementport is supposed to be 10.128.1.2 not 0.2 which leads to policy not translating well into SBDB flows.

Also fixes: #4723

[tssurya]

cc @maiqueb / @trozet / @dceara

[tssurya]

unrelated flake: https://github.com/ovn-org/ovn-kubernetes/actions/runs/10809609667/job/29985084566?pr=4718

2024-09-11T10:33:33.3230602Z �[91m�[1m[Fail] �[0m�[90mOVN master EgressIP Operations �[0m�[0mWatchEgressNodes running with WatchEgressIP �[0m�[91m�[1m[It] should re-balance EgressIPs when their node is removed �[0m
2024-09-11T10:33:33.3232456Z �[37m/home/runner/work/ovn-kubernetes/ovn-kubernetes/go-controller/pkg/ovn/egressip_test.go:11879�[0m

[dceara]

@tssurya don't we need the exact same thing for the secondary L2 network controller?

Also, I'm not sure why the return value of

// Subnets returns the Subnets value
func (nInfo *secondaryNetInfo) Subnets() []config.CIDRNetworkEntry {
	return nInfo.subnets
}

isn't exactly the right thing we need?

Do we need to fix this instead?

func newLayer3NetConfInfo(netconf *ovncnitypes.NetConf) (NetInfo, error) {
	subnets, _, err := parseSubnets(netconf.Subnets, "", types.Layer3Topology)
	if err != nil {
		return nil, err
	}
	joinSubnets, err := parseJoinSubnet(netconf.JoinSubnet)
	if err != nil {
		return nil, err
	}
	ni := &secondaryNetInfo{
		netName:        netconf.Name,
		primaryNetwork: netconf.Role == types.NetworkRolePrimary,
		topology:       types.Layer3Topology,
		subnets:        subnets,
		joinSubnets:    joinSubnets,
		mtu:            netconf.MTU,
		nadNames:       sets.Set[string]{},
	}
	ni.ipv4mode, ni.ipv6mode = getIPMode(subnets)
	return ni, nil
}

Do we need a unit test?

Thanks,
Dumitru

[maiqueb]

Couldn't we update the node annotation used in the unit tests in such a way we would see this change in the unit tests ? i.e. this extra route would be installed ?

Having the unit test reproduce the "real" scenario as close as possible would be beneficial IMHO.

[tssurya]

pretty sure the fact that unit tests for this didn't fail is because this annotation is already present. I can double check to see what's the clustersubnet used v/s node subnet

[tssurya]

@dceara for L2 the subnet is nad's subnet cause its a flat L2 we don't have a per nodesubnet we use there like we do for L3, hence the distinction. L3 should look more like our default one where we use the same method that I am changing in this PR, hope that makes sense

[maiqueb]

pretty sure the fact that unit tests for this didn't fail is because this annotation is already present. I can double check to see what's the clustersubnet used v/s node subnet

So in the unit tests we are already installing the local pod subnet route, which this PR says it's missing in production ?

[dceara]

@dceara for L2 the subnet is nad's subnet cause its a flat L2 we don't have a per nodesubnet we use there like we do for L3, hence the distinction. L3 should look more like our default one where we use the same method that I am changing in this PR, hope that makes sense

Ah I see, ok disregard that part.

[tssurya]

pretty sure the fact that unit tests for this didn't fail is because this annotation is already present. I can double check to see what's the clustersubnet used v/s node subnet

So in the unit tests we are already installing the local pod subnet route, which this PR says it's missing in production ?

right so the problem is not in NBDB level its at SBDB level. northd complains that the prefix we are providing is wrong

see my PR description: No path for static route 10.128.0.0/24; next hop 100.88.0.2 so this should have been 10.128.0.1/24 which is the subnet for this node, so the best we can do with UTs is to change the distinction on the prefix used. my guess is we are using the same thing as cluster subnet and node subnet and so nothing changed for tests.. I will try to sort the test part out.

[tssurya]

OK! Finally CI looks good. let me see if I can fix the unit tests here ... or at least figure out why existing unit tests didn't fail for these

[trozet]

I provided some context here to what is wrong with the unit tests:
#4723 (comment)

I cant really condone merging this PR without fixing those unit tests first.

[tssurya]

I provided some context here to what is wrong with the unit tests: #4723 (comment)

I cant really condone merging this PR without fixing those unit tests first.

I agree: #4718 (comment) I was going to look into the tests as part of this PR itself See my above two comments. So +1 I agree with UT coverage that you, miguel and dumitru have asked for.. working on it. Apologies for the delay.

I was looking for a double check/review on the routes/policies changes I am making to see if it made sense since its touching some crucial pieces before I start on test journey..

Also just to leave notes around the /16 route that route is owned by EIP/ESVC and L3 Migration code: CreateDefaultRouteToExternal is the util that creates this.

So while the gateway creates the per node route the catch-all route exists as an umbrella for those features... so that one is not supposed to be created from gateway code. @martinkennelly will add it as part of EIP Support on UDN

[tssurya]

@maiqueb / @dceara / @trozet : I have fixed the tests to be more explicit, PTAL

[tssurya]

another unrelated EIP flake: #4724

[tssurya]

failures in multi-homing are unrelated: we had a regression merge into multus last night
@jcaamano is fixing it in 9746662

[trozet]

thanks @tssurya !