Support creation of GCE_VM_PRIMARY_IP NEGs for L4 ILB Services. #959

prameshj · 2019-12-02T19:27:48Z

This change adds support in transaction syncer to create GCE_VM_PRIMARY_IP NEGs.

The neg controller has 2 new flags - one for watching ingress and one for services.
LoadBalancer services will be processed only if the second flag is enabled. This is disabled by default and can be enabled by adding "ILBSubsets" Alpha feature gate.

/assign @freehan @bowei

freehan · 2019-12-10T22:47:01Z

pkg/neg/manager.go

@@ -316,6 +338,9 @@ func getSyncerKey(namespace, name string, servicePortKey negtypes.PortInfoMapKey
 	if flags.F.EnableNonGCPMode {
 		networkEndpointType = negtypes.NonGCPPrivateEndpointType
 	}
+	if portInfo.PortTuple.Port == 0 && portInfo.PortTuple.Name == string(negtypes.VmPrimaryIpEndpointType) {


check if the PortTuple is empty to determine if it is VM_PRIMARY_IP instead of using the name

freehan · 2019-12-10T23:05:15Z

pkg/neg/types/types.go

-			NegName:       namer.NEG(namespace, name, svcPortTuple.Port),
-			ReadinessGate: readinessGate,
+			PortTuple:          svcPortTuple,
+			NegName:            namer.NEG(namespace, name, svcPortTuple.Port),


Create a new namer func for VM_PRIMARY_IP NEG type with format k8s1-clusterhash-namespace-name-hash

Create a new func similar to NewPortInfoMap to create PortInfoMap for this case.

freehan · 2019-12-10T23:16:31Z

pkg/neg/syncers/transaction.go

 	recorder       record.EventRecorder
 	cloud          negtypes.NetworkEndpointGroupCloud
 	zoneGetter     negtypes.ZoneGetter

+	// randomize indicates that the endpoints of the NEG can be picked at random, rather
+	// than following the endpoints of the service. This only applies in the GCE_VM_PRIMARY_IP NEG


put
"This only applies in the GCE_VM_PRIMARY_IP NEG" in front

freehan · 2019-12-10T23:19:35Z

pkg/neg/syncers/transaction.go

 	}

 	// Merge the current state from cloud with the transaction table together
 	// The combined state represents the eventual result when all transactions completed
 	mergeTransactionIntoZoneEndpointMap(currentMap, s.transactions)
 	// Calculate the endpoints to add and delete to transform the current state to desire state
 	addEndpoints, removeEndpoints := calculateNetworkEndpointDifference(targetMap, currentMap)
+	if s.randomize && len(removeEndpoints) > 0 {
+		// Make removals minimum since the traffic will be abruptly stopped. Log removals
+		klog.Infof("Removing endpoints %+v from GCE_VM_PRIMARY_IP NEG %s", removeEndpoints, s.negName)


need a log level

freehan · 2019-12-10T23:21:23Z

pkg/neg/syncers/transaction.go

@@ -156,7 +177,9 @@ func (s *transactionSyncer) syncInternal() error {
 	// filter out the endpoints that are in transaction
 	filterEndpointByTransaction(committedEndpoints, s.transactions)

-	s.commitPods(committedEndpoints, endpointPodMap)
+	if s.NegSyncerKey.NegType != negtypes.VmPrimaryIpEndpointType {


try not to skip it.
Just explore if it is okay to leave it.

Yes this is ok to leave as is without the if check. Will make the change.

freehan · 2019-12-19T01:36:07Z

pkg/neg/syncers/transaction.go

+
+	switch {
+	case s.NegSyncerKey.NegType == negtypes.VmPrimaryIpEndpointType:
+		nodeLister := listers.NewNodeLister(s.nodeLister)


why do you need a NewNodeLister here?

This is to select nodes at random for the NEG backing an "ExternalTrafficPolicy: Cluster" ilb.

store NodeLister instead of the Cache.Indexer

The utils_test.go uses the Cache.Indexer in order to add nodes and test the transaction functions. Leaving it as is for now. Please let me know if you have ideas on how to continue testing this.

freehan · 2019-12-20T18:24:43Z

pkg/neg/syncers/utils.go

@@ -169,6 +170,42 @@ func ensureNetworkEndpointGroup(svcNamespace, svcName, negName, zone, negService
 	return nil
 }

+func toZonePrimaryIPEndpointMap(endpoints *apiv1.Endpoints, nodeLister listers.NodeLister, zoneGetter negtypes.ZoneGetter, randomize bool, currentMap map[string]negtypes.NetworkEndpointSet, serviceKey string) (map[string]negtypes.NetworkEndpointSet, error) {


add comment

freehan · 2019-12-20T18:27:28Z

pkg/neg/syncers/subsets.go

+	return subset
+}
+
+func getSubsetPerZone(nodes []*v1.Node, zoneGetter negtypes.ZoneGetter, svcID string, currentMap map[string]negtypes.NetworkEndpointSet, newEpCount int, randomize bool) (map[string]negtypes.NetworkEndpointSet, error) {


add comment

freehan · 2019-12-20T18:28:44Z

pkg/neg/syncers/subsets.go

+	return result, nil
+}
+
+func getSubsetCount(currentCount, newCount, numZones int, randomize bool) int {


add comment

freehan · 2019-12-20T18:30:54Z

pkg/neg/syncers/utils.go

+		nodes, _ := nodeLister.ListWithPredicate(utils.GetNodeConditionPredicate())
+		return getSubsetPerZone(nodes, zoneGetter, serviceKey, currentMap, utils.NumEndpoints(endpoints), true)
+	}
+	nodes := []*v1.Node{}


add comment here to describe the outcome:

basically pick the nodes where the endpionts are located.

freehan · 2019-12-20T18:32:17Z

pkg/neg/syncers/subsets.go

+// If the input list is smaller than the desired subset count, the entire list is returned. The hash salt
+// is used so that a different subset is returned even when the same node list is passed in, for a different salt value.
+// It also keeps the subset relatively stable for the same service.
+func PickSubsetsNoRemovals(nodes []*v1.Node, salt string, count int, current []negtypes.NetworkEndpoint) []*v1.Node {


no need to export this function, right?

Only used in this package

Also, the function name is a bit confusing. PickSubsetsNoRemovals seems to indicate No removal. But it actually is minimal removal? Or minimal change.

That's correct, changed the name and not exported.

freehan · 2019-12-20T18:44:24Z

pkg/neg/syncers/subsets.go

+	return hex.EncodeToString(hashSum[:])
+}
+
+// PickSubsetsNoRemovals ensures that there are no node removals from current subset unless the node no longer exists.


add some examples in comment

freehan · 2019-12-20T18:48:55Z

pkg/neg/syncers/subsets.go

+			}
+		}
+	}
+	if len(subset) >= count {


add comment remove excessive nodes

freehan · 2019-12-20T18:49:57Z

pkg/neg/syncers/subsets.go

+)
+
+// NodeInfo stores node metadata used to sort nodes and pick a subset.
+type NodeInfo struct {


comment each field

freehan · 2019-12-20T18:51:04Z

pkg/neg/syncers/subsets.go

+	// Pick all nodes from existing subset if still available.
+	for _, ep := range current {
+		for _, nodeInfo := range info {
+			curHashName := getHashedName(ep.Node, salt)


calculate this in the outter for loop?

good catch, done.

freehan · 2019-12-20T19:00:31Z

pkg/neg/syncers/subsets.go

+		return info[i].hashedName < info[j].hashedName
+	})
+	// Pick all nodes from existing subset if still available.
+	for _, ep := range current {


I am wondering if you can make this easier if you use maps. (1map[hashedName]NetworkEndpoint, another map[hashedName]NodeInfo

After you picked the existing ones, the left over ones can be sorted and picked. Not sure if this is good enough.

that can be done.. but I am not sure if it simplifies it that much.
I can use a map for the current subset, another map for the current nodes - map[nodeName]NodeInfo.
Picking existing nodes becomes easier. It does get rid of the "skip" boolean as nodes can be deleted from the NodeInfo map as they are picked.
But we need translation from map to slice before sorting as well as translating current subset slice to map as well.

freehan · 2019-12-20T23:13:41Z

pkg/neg/syncers/transaction.go

@@ -130,21 +140,32 @@ func (s *transactionSyncer) syncInternal() error {
 		return nil
 	}

-	targetMap, endpointPodMap, err := toZoneNetworkEndpointMap(ep.(*apiv1.Endpoints), s.zoneGetter, s.PortTuple.Name, s.podLister, s.NegSyncerKey.SubsetLabels, s.NegSyncerKey.NegType)
+	currentMap, err := retrieveExistingZoneNetworkEndpointMap(s.negName, s.zoneGetter, s.cloud, s.NegSyncerKey.GetAPIVersion())


Move mergeTransactionIntoZoneEndpointMap after this. That merges the currentMap with the ongoing transactions (NEG API calls in flight). Then the result is the end state after all transactions are done.

I am using the end result of mergeTransactionIntoZoneEndpointMap to calculate the subset, so we use the result of all pending transactions being applied. Do we want to do something else?

Resolved offline, this is the correct behavior.

freehan · 2020-01-14T23:46:59Z

pkg/utils/utils.go

+}
+
+// IsLegacyL4ILBService returns true if the given LoadBalancer service is managed by service controller.
+func IsLegacyL4ILBService(g *gce.Cloud, svc *api_v1.Service) bool {


gce.Cloud is not used, correct?

correct, fixed.

freehan · 2020-01-14T23:47:46Z

pkg/neg/controller.go

@@ -67,6 +74,7 @@ type Controller struct {
 	ingressLister               cache.Indexer
 	serviceLister               cache.Indexer
 	client                      kubernetes.Interface
+	gceCloud                    *gce.Cloud


I do not think this is needed.

correct, fixed.

freehan · 2020-01-14T23:51:15Z

pkg/neg/types/types.go

@@ -177,6 +206,7 @@ func (p1 PortInfoMap) Merge(p2 PortInfoMap) error {
 		mergedInfo.NegName = portInfo.NegName
 		// Turn on the readiness gate if one of them is on
 		mergedInfo.ReadinessGate = mergedInfo.ReadinessGate || portInfo.ReadinessGate
+		mergedInfo.RandomizeEndpoints = mergedInfo.RandomizeEndpoints || portInfo.RandomizeEndpoints


I think this should be an error instead of a merge.

I do not think a service should ever has this 2 states .

freehan · 2020-01-15T00:07:09Z

pkg/neg/syncers/subsets.go

+	// Max number of subsets in ExternalTrafficPolicy:Local
+	maxSubsetSizeLocal = 250
+	// Max number of subsets in ExternalTrafficPolicy:Cluster, which is the default mode.
+	maxSubsetSizeDefault = 25


just a recommendation. consider 24

I agree, 24 is a better number for equally splitting between 2, 3, 4 zones. We will have the same split for 25 too, since we do integer division.

freehan · 2020-01-15T00:10:17Z

pkg/neg/syncers/endpoints_calculator.go

+
+// LocalL4ILBEndpointGetter implements methods to calculate Network endpoints for VM_PRIMARY_IP NEGs when the service
+// uses "ExternalTrafficPolicy: Local" mode.
+type LocalL4ILBEndpointsCalculator struct {


add comment to say which interface it implementations.
and the corresponding algorithm and example.

freehan · 2020-01-15T00:11:03Z

pkg/neg/syncers/endpoints_calculator.go

+// CalculateEndpoints determines the endpoints in the NEGs based on the current service endpoints and the current NEGs.
+func (l *LocalL4ILBEndpointsCalculator) CalculateEndpoints(ep *v1.Endpoints, currentMap map[string]types.NetworkEndpointSet) (map[string]types.NetworkEndpointSet, types.EndpointPodMap, error) {
+	// List all nodes where the service endpoints are running. Get a subset of the desired count.
+	nodeZoneMap := make(map[string][]*v1.Node)


nit: zoneNodeMap?

freehan · 2020-01-15T00:18:52Z

pkg/neg/syncers/endpoints_calculator.go

+	numZones := len(nodeZoneMap)
+	perZoneCount := l.getPerZoneSubsetCount(numZones, numEndpoints)
+	// Compute the networkEndpoints, with endpointSet size in each zone being atmost `perZoneCount` in size
+	subsetMap, err := getSubsetPerZone(nodeZoneMap, perZoneCount, l.svcId, currentMap)


I am wondering if the following case would happen?

OnlyLocal service with endpoints in 3 zone. Zone a has 240 endpoints, zone b/c each has 1 endpoint.
Will it put 80 endpoints in NEG in zone a and 1 endpoint in zone b and c?

I think getSubsetPerZone and pickSubsetsMinRemovals need some reorganizing to solve this problem.

Just a thought:
given inputs:

MAX_ENDPOINT_IN_SUBSET either 25 or 250.

candidates nodes

current map

correct, this can happen. I had added a TODO to fix this in a follow up, I will added it back. It can be fixed by picking upto limit/n nodes, but selecting more than that if other zones had fewer nodes. I can try to include that change in this same PR or in a follow up.

freehan · 2020-01-15T00:42:58Z

pkg/neg/syncers/transaction.go

@@ -88,6 +100,19 @@ func NewTransactionSyncer(negSyncerKey negtypes.NegSyncerKey, networkEndpointGro
 	return syncer
 }

+func getEndpointsCalculator(syncer *transactionSyncer) negtypes.NetworkEndpointsCalculator {


may be consider initialize EndpointsCalculator in the SyncerManager?
so that randomize is no longer needed.

freehan · 2020-01-15T00:44:04Z

pkg/utils/namer/namer.go

@@ -463,6 +463,14 @@ func (n *Namer) NEGWithSubset(namespace, name, subset string, port int32) string
 	return fmt.Sprintf("%s-%s-%s-%s-%s-%s", n.negPrefix(), truncNamespace, truncName, truncPort, truncSubset, negSuffix(n.shortUID(), namespace, name, portStr, subset))
 }

+func (n *Namer) PrimaryIPNEG(namespace, name string) string {


add comment.

LoadBalancer services will be processed only if the runL4 flag is true. This is false by default. The neg controller also watches Nodes if runL4 is enabled. The subsetting logic minimizes node removals from Endpoint Group to avoid disruption to existing LB sessions.

Generated using "go mod vendor"

Added a controller test to ensure syncers created. Also added unit tests for newly added utils.

freehan

/lgtm
/approve

k8s-ci-robot · 2020-01-16T23:23:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot assigned bowei and freehan Dec 2, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 2, 2019

k8s-ci-robot requested review from rramkumar1 and thockin December 2, 2019 19:28

freehan reviewed Dec 10, 2019

View reviewed changes

freehan reviewed Dec 19, 2019

View reviewed changes

freehan reviewed Dec 20, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 31, 2019

prameshj force-pushed the negl4ilb branch from d842540 to 81b9f11 Compare December 31, 2019 01:23

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 31, 2019

prameshj force-pushed the negl4ilb branch from f76730d to fca8565 Compare January 6, 2020 07:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2020

prameshj force-pushed the negl4ilb branch 6 times, most recently from dee780b to 202eba3 Compare January 9, 2020 21:16

prameshj mentioned this pull request Jan 13, 2020

Add a controller for handling L4 Internal LoadBalancer services #991

Merged

freehan reviewed Jan 15, 2020

View reviewed changes

prameshj force-pushed the negl4ilb branch 2 times, most recently from 40eec7f to d0a8d85 Compare January 16, 2020 01:11

prameshj added 3 commits January 15, 2020 22:31

Autogenerated vendor changes.

376e173

Generated using "go mod vendor"

Unit tests to verify the new L4 mode.

112a1b1

Added a controller test to ensure syncers created. Also added unit tests for newly added utils.

prameshj force-pushed the negl4ilb branch from d0a8d85 to 112a1b1 Compare January 16, 2020 06:40

freehan approved these changes Jan 16, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2020

k8s-ci-robot merged commit c0fef63 into kubernetes:master Jan 16, 2020

Support creation of GCE_VM_PRIMARY_IP NEGs for L4 ILB Services. #959

Support creation of GCE_VM_PRIMARY_IP NEGs for L4 ILB Services. #959

Conversation

prameshj commented Dec 2, 2019

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

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

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

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

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 16, 2020