Allocate tunnel IDs to pods attached to layer2 networks with interconnect #3673

jcaamano · 2023-06-14T14:31:01Z

Layer 2 networks on interconnect require that we set a tunnel ID to the logical switch ports attached to the network switch that effectively becomes a transit switch.

This ID is allocated by cluster manager with the same life cycle of the IP allocation that we already had for localnet networks, now also for layer2 networks, so it makes sense to make this ID part of the PodAnnotation.

jcaamano · 2023-06-14T14:32:50Z

Note this depends on #3620 so includes those commits. The intention is to advance with reviews here while that one is merged.

First commit of this PR is Export ID allocator interface

@maiqueb PTAL

maiqueb · 2023-06-14T15:03:19Z

/cc

coveralls · 2023-06-14T15:20:13Z

coverage: 52.988% (-0.4%) from 53.417% when pulling 2a880c0 on jcaamano:ic-l2-id-allocator into c5b7e25 on ovn-org:master.

jcaamano · 2023-06-20T09:31:34Z

/retest-failed

trozet

I didn't do a really in depth review here, but looking at the gist of it I think it looks good. No major issues with this PR with regards to the layer 2 network commits.

maiqueb

Look good.

I just ask for some code to be squashed, and would rename the ID attribute to something more explicit, because I worry ID is too generic.

Thank you for this PR.

maiqueb · 2023-06-23T11:49:34Z

go-controller/pkg/allocator/id/allocator.go

can this commit maybe be squashed into the one where this struct is used ? If I have the usages, it becomes easier to understand how this is going to be used.

What struct? What commit?

I meant commit 181cf96c14f1f354cca99b8f63d1cbf26c69bab5 - named Move ID allocator to allocator package.

Like, I see this being moved from the clustermanager package into a new allocator/id package, but I don't see the new usage, thus I don't get why this is needed.

I guess that it will be used in commit 4d7e74b6444f68cde47583712e97fc63bd9bdb10, whose title is Allocate an tunnel ID with the PodAnnotation.

IMO it would still be IMO clearar if you squashed those, but this is just a nit.

I get that you miss the reasoning and I added a commit message with an explanation.

Yes, I read it. It makes sense, and is helpful.

I'm just saying actually seeing the usage from outside the package is more helpful than deriving meaning from a commit msg.

No need to follow-through, I've already understood what is happening.

I know that it would be easier to understand but that can lead to bigger commits. So I compromised here. I did that in the past restructuring packages, etc...

maiqueb · 2023-07-04T08:56:00Z

go-controller/pkg/util/pod_annotation.go

@@ -71,6 +71,9 @@ type PodAnnotation struct {
 	Gateways []net.IP
 	// Routes are additional routes to add to the pod's network namespace
 	Routes []PodRoute
+
+	// ID assigned to each pod for L2 secondary networks
+	ID int


if this attribute is only useful for a particular type of network, I think its name should somehow reflect that. Maybe FlatL2TunnelID ?

Another question: here you specifically mean flatL2 topology, right ? Localnet does not require this, right ?

Renamed to TunnelID. I don't want to be more specific than that.

Reworded the comment as well. It's for layer2 topologies.

maiqueb · 2023-07-04T08:58:43Z

go-controller/pkg/util/pod_annotation.go

+// derived from the allocator provided IPs. If the requested IPs cannot be
+// honored, a new set of IPs will be allocated unless reallocateIP is set to
+// false.
+func AllocatePodAnnotationWithID(


I worry ID is too generic. I think this should somehow hint it should be used from a particular topology type.

Renamed to AllocatePodAnnotationWithTunnelID. I don't really want to be more specific than that just in case is used for other things in the future.

maiqueb · 2023-07-04T09:05:37Z

go-controller/pkg/clustermanager/network_cluster_controller.go

I don't understand the purpose of this commit; isn't PodIPAllocator more concrete / explicit than PodAllocator ?

It is, but since we are going to be allocating IDs as well. I opted for the more generic name PodAllocator

Yes ... But I would guess a PodAllocator to allocate pods, not IPs for pods. On the other hand, I would expect a PodIPAllocator to allocate IPs for pods - which is what this does.

Just saying I think there are advantages in being more explicit.

I can take suggestions for better names, but having globally good names is not always easy to come by. Naming has a context which does not require to come up with globally good names. The PodAllocator has the context of operating within ovn-kubernets so it is obviously not allocating pods. It has documentation attached explaining what it does.

maiqueb · 2023-07-04T09:07:05Z

go-controller/pkg/clustermanager/pod/allocator.go

+	hasIDAllocation := util.DoesNetworkRequireIDs(a.netInfo)
+
+	if !hasIPAM && !hasIDAllocation {
+		// nothing to do


nit: redundant. I learn nothing from this comment.

maiqueb · 2023-07-04T09:09:17Z

go-controller/pkg/allocator/id/allocator.go

@@ -26,7 +26,7 @@ type IDAllocator struct {

 // NewIDAllocator returns an IDAllocator
 func NewIDAllocator(name string, maxIds int) (*IDAllocator, error) {
-	idBitmap := bitmapallocator.NewContiguousAllocationMap(maxIds, name)
+	idBitmap := bitmapallocator.NewRoundRobinAllocationMap(maxIds, name)


can this be squashed into the commit that introduced this code ?

Like, having this as a separate commit raises the question of what changed, and I guess nothing did: you could have created the ID allocator with this strategy directly.

I didn't introduce this code. I will add to the commit message the reason I did this change.

jcaamano · 2023-07-05T20:29:33Z

Added commit messages that should hopefully give more clarity to the changes.

@maiqueb PTAL

maiqueb · 2023-07-06T08:40:27Z

go-controller/pkg/clustermanager/pod/allocator.go

+	releasedPodsMutex sync.Mutex
+}
+
+// NewPodAllocator builds a new PodIPAllocator


this comment is outdated, isn't it ?

FWIW, I would still call this neat struct PodIPAllocator though.

This is the doc attached with the PodAllocator:

// PodAllocator acts on pods events handed off by the cluster network controller // and allocates or releases resources (IPs and tunnel IDs at the time of this // writing) to pods on behalf of cluster manager.

I don't think PodIPAllocator fits. Can take other suggestions. I thought about PodResourceAllocator but I didn't like it specially, as Resource can give the impression of referring to kubernetes resources, but I can bend here.

I think that Resource is too generic and makes the reader think about kubernetes resources.

Let's leave it as is then.

maiqueb

Thank you.

dcbw · 2023-07-08T14:15:27Z

@jcaamano I would almost merge the first 6 commits today... any chance you could split those out? (through Annotation errors compatible with errors pkg). Big +1 on removing the LS UUID cache.

trozet · 2023-07-10T22:07:58Z

go-controller/pkg/clustermanager/secondary_network_cluster_manager.go

@@ -114,6 +114,8 @@ func (sncm *secondaryNetworkClusterManager) isTopologyManaged(nInfo util.NetInfo
 	switch nInfo.TopologyType() {
 	case ovntypes.Layer3Topology:
 		return true
+	case ovntypes.Layer2Topology:
+		return config.OVNKubernetesFeature.EnableInterconnect


why is this decided based on if interconnect is enabled for not? If not IC, then allocation is handled somewhere else? where is that done?

That's done by ovnkube-master, in (bnc *BaseNetworkController) allocatePodAnnotationForSecondaryNetwork. There we have a reciprocal check.

I will add comments here though.

trozet · 2023-07-10T22:11:11Z

why is layer2 network e2es enabled in this PR? going to be another PR?

jcaamano · 2023-07-11T07:58:13Z

why is layer2 network e2es enabled in this PR? going to be another PR?

I guess I made a mistake when rebasing #3609

Commit Enable multi-homing e2e tests for localnet with IC, which is enabling localnet e2e tests, should have gone in the localnet PR.

I will make a separate PR for it.

edit: #3757

The ID allocator will be used form different packages. Lets add an interface to export its functionality and for easy mocking. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

So that it can be used to allocate tunnel IDs for pods from the clustermanager/pod package. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Tunnel IDs will be allocated for a specific network from a context that should not affect allocation of other networks. Let's protect against this with an interface bounded to a specific network. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Pods attached to secondary networks with layer2 topologies on interconnect require a tunnel ID allocated to be configured as tunnel keys on its logical switch ports of the transit switch. Allocate this tunnel Id in the PodAnnotation as it shares the same lifecycle as the other data stored there. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Since it will now allocate tunnel IDs as well and not just IPs. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Pods attached to secondary networks with layer2 topologies on interconnect require a tunnel ID allocated to be configured as tunnel keys on its logical switch ports of the transit switch. This add support to allocate such IDs from cluster manager. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Tunnel IDs allocated to pods have a more dynamic lifecycle than IDs allocated to networks. Lets change the allocation strategy to round robin to reduce the posibility of potential collisions when a de-allocated tunnel ID is allocated to a pod while still being used by one of the zones that might have not yet process the original pod termination event. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

This will enable cluster manager to allocate the PodAnnotation to pods on interconnect instead of the zone controllers. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

jcaamano requested review from trozet, dcbw and girishmg as code owners June 14, 2023 14:31

jcaamano force-pushed the ic-l2-id-allocator branch 6 times, most recently from d17c88d to 7290144 Compare June 20, 2023 09:03

jcaamano force-pushed the ic-l2-id-allocator branch 4 times, most recently from 5ac2f7c to 6025ebd Compare June 27, 2023 15:26

trozet reviewed Jun 30, 2023

View reviewed changes

maiqueb reviewed Jul 4, 2023

View reviewed changes

jcaamano force-pushed the ic-l2-id-allocator branch from 6025ebd to 89b6b81 Compare July 5, 2023 20:29

jcaamano force-pushed the ic-l2-id-allocator branch from 89b6b81 to 5f6763e Compare July 6, 2023 08:13

maiqueb reviewed Jul 6, 2023

View reviewed changes

maiqueb approved these changes Jul 6, 2023

View reviewed changes

jcaamano force-pushed the ic-l2-id-allocator branch 2 times, most recently from 30f5daa to e3d6369 Compare July 6, 2023 10:42

jcaamano force-pushed the ic-l2-id-allocator branch 2 times, most recently from 2a880c0 to 41bf4bb Compare July 10, 2023 10:25

trozet force-pushed the ic-l2-id-allocator branch from 41bf4bb to fb78549 Compare July 10, 2023 21:57

trozet reviewed Jul 10, 2023

View reviewed changes

jcaamano force-pushed the ic-l2-id-allocator branch from fb78549 to 6c3d49b Compare July 11, 2023 08:28

jcaamano added 7 commits July 11, 2023 08:51

Add ID Allocator interface

9ad0479

The ID allocator will be used form different packages. Lets add an interface to export its functionality and for easy mocking. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Move ID allocator to allocator package

cc23a1c

So that it can be used to allocate tunnel IDs for pods from the clustermanager/pod package. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Add Named ID allocator

e7763f1

Tunnel IDs will be allocated for a specific network from a context that should not affect allocation of other networks. Let's protect against this with an interface bounded to a specific network. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Rename PodIPAllocator to PodAllocator

cffc137

Since it will now allocate tunnel IDs as well and not just IPs. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

jcaamano force-pushed the ic-l2-id-allocator branch from 6c3d49b to a36b754 Compare July 11, 2023 08:53

jcaamano changed the title ~~Allocate IDs to pods attached to layer2 networks with interconnect~~ Allocate tunnel IDs to pods attached to layer2 networks with interconnect Jul 11, 2023

Enable cluster manager support for layer2 networks

fd07c04

This will enable cluster manager to allocate the PodAnnotation to pods on interconnect instead of the zone controllers. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

jcaamano force-pushed the ic-l2-id-allocator branch from a36b754 to fd07c04 Compare July 11, 2023 09:05

trozet approved these changes Jul 11, 2023

View reviewed changes

trozet merged commit f069ee4 into ovn-org:master Jul 11, 2023
25 checks passed

Allocate tunnel IDs to pods attached to layer2 networks with interconnect #3673

Allocate tunnel IDs to pods attached to layer2 networks with interconnect #3673

Conversation

jcaamano commented Jun 14, 2023

jcaamano commented Jun 14, 2023

maiqueb commented Jun 14, 2023

coveralls commented Jun 14, 2023 • edited Loading

jcaamano commented Jun 20, 2023

trozet left a comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcaamano commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

dcbw commented Jul 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trozet commented Jul 10, 2023

jcaamano commented Jul 11, 2023 • edited Loading

coveralls commented Jun 14, 2023 •

edited

Loading

maiqueb Jul 6, 2023 •

edited

Loading

jcaamano commented Jul 11, 2023 •

edited

Loading