Multi networking neg controller #2023

mmamczur · 2023-03-17T15:58:58Z

Support for multi-networking in the NEG controller.

k8s-ci-robot · 2023-03-17T16:00:08Z

Hi @mmamczur. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

swetharepakula · 2023-03-17T18:41:23Z

/ok-to-test

aojea · 2023-04-08T10:34:44Z

pkg/neg/controller.go

@@ -572,8 +589,12 @@ func (c *Controller) mergeVmIpNEGsPortInfo(service *apiv1.Service, name types.Na
 	onlyLocal := helpers.RequestsOnlyLocalTraffic(service)
 	// Update usage metrics.
 	negUsage.VmIpNeg = usageMetrics.NewVmIpNegType(onlyLocal)
+	networkInfo, err := multinetwork.ServiceNetwork(service, c.networkLister, c.gkeNetworkParamSetLister, c.cloud)


so, this will be a noop if --multi-network flag is not enabled

yes, that was the point, nil means default network

I'm thinking of redoing this. Always populating this network info, with default network URLs if no multinetworking is needed. Would be nicer than having nil checks everywhere.

I reworked this
with the current version when --multi-network flag is not enabled the NetworkInfo should be present anyway but will contain the default network information.

aojea · 2023-04-08T10:36:43Z

pkg/multinetwork/multinetwork.go

+	networkingGKEGroup     = "networking.gke.io"
+	gkeNetworkParamSetKind = "gkenetworkparamset"
+	networkSelector        = "networking.gke.io/network"


it these things are defined in some place and are official we should export them on the origin , these are official APIs, otherwise there is a risk of drifting defining it in different places

I tried to replicate whatever way this is done in our other multinet code. for selector a constant named NetworkAnnotationKey is used (a bit confusing but ok). Seems that there is no good constant for GKENetworkParamSet exported anywhere. Not sure, should we add it upstream?

cezarygerard

I am reviewing network_test.go yet

pkg/neg/syncers/endpoints_calculator_test.go

pkg/neg/syncers/subsets.go

cezarygerard · 2023-04-27T14:01:55Z

pkg/neg/types/cloudprovideradapter.go

@@ -112,3 +112,11 @@ func (a *cloudProviderAdapter) NetworkURL() string {
 func (a *cloudProviderAdapter) SubnetworkURL() string {
 	return a.subnetworkURL
 }
+
+func (a *cloudProviderAdapter) NetworkProjectID() string {
+	return a.c.NetworkProjectID()


maybe they meant a for adapter?

I don't know what is the approach, we can also have 'cpa', should we change all these to 'c'?

"cpa" would be a good name for receiver (as for golang)
"cloud" instead of c" would a better field name

just mark it resolved, let's not jump into refactoring-rabbit-hole while implementing new features

pkg/neg/types/fakes.go

pkg/network/network.go

pkg/network/network_test.go

cezarygerard · 2023-04-27T17:19:32Z

pkg/network/network.go

+// All nodes are connected to the default network.
+// For non default networks the result is based on the data from
+// the 'networking.gke.io/north-interfaces' node annotation.
+func (ni *NetworkInfo) NodeConnected(node *apiv1.Node) bool {


comment regarding the NetworkInfo struct

isnt; it better to have positive IsDefault or Default than negated field name IsNonDefault?

the reason I did this is that false is the default when you don't specify any value, you can do ni := NetworkInfo{}
and it will act as default
Then again it won't know the actual Network/subnetwork URLs so that's not great either

for this case
ni := NetworkInfo{}
you can define a variable in the network package

var DefaultNetworkInfo = &NetworkInfo{ IsDefault: true, K8sNetwork: networkv1.DefaultPodNetworkName, }

and use
L4ClusterEndpointCalculator := NewClusterL4ILBEndpointsCalculator(listers.NewNodeLister(nodeLister), zoneGetter, svcKey, klog.TODO(), network.DefaultNetworkInfo)
instead of
L4ClusterEndpointCalculator := NewClusterL4ILBEndpointsCalculator(listers.NewNodeLister(nodeLister), zoneGetter, svcKey, klog.TODO(), &network.NetworkInfo{})

or you can create a method for the same purpose

func DefaultNetworkInfo() *NetworkInfo{ return &NetworkInfo{ IsDefault: true, K8sNetwork: networkv1.DefaultPodNetworkName, } }

oh, there is such method....
but requires cloudProvider arg

I still prefer positive variable IsDefault or just Default

ok I changed the bool to IsDefault instead of IsNonDefault

cezarygerard · 2023-04-27T17:26:49Z

pkg/network/network.go

+	}, nil
+}
+
+func defaultNetwork(cloudProvider cloudNetworkProvider) *NetworkInfo {


does cloudProvider ever change?
or this returns same NetworkInfo every time?

if the latter is true then you can store NetworkInfo in private var in this file and only initialize it at first execution of this func

you don't even need to worry about race when initializing this var (as in singleton pattern), initializing multiple times will break nothing (same NetworkInfo struct)

there are 2 actual structs that can be used here:

cloudProviderAdapter from pkg/neg/types/cloudprovideradapter.go - used in NEG controller

Cloud imported cloud-provider-gcp - used in the ILB controller

I wanted this code to work with both

as for the values, I don't know if they change tbh, we could do lazy init probably.
Previously we never stored the network and subnetwork URL, always took them from these Clouds

cezarygerard · 2023-04-27T17:28:08Z

pkg/network/network_test.go

+			},
+			want: false,
+		},
+		{


this case:

{ desc: "2 always connected to the default network", networkInfo: NetworkInfo{ IsNonDefault: true, K8sNetwork: "default", }, node: &apiv1.Node{ ObjectMeta: metav1.ObjectMeta{Name: "test-node", Annotations: map[string]string{}}, }, want: false, },

would pass either

which feels weird

yea, it's made so that whenever IsNonDefault is set to true then there is multinet logic involved, even if the name is set to "default".
Having the flag in addition to name is convenient since you can create empty NetworkInfo and it will represent default (but not always so this is tricky)

bowei · 2023-05-04T16:26:43Z

/hold

/assign @swetharepakula

swetharepakula

This mostly looks good to me. A few nits and cleanup comments.

Can we separate out the vendor changes into its own commit?

pkg/neg/syncers/endpoints_calculator_test.go

swetharepakula · 2023-05-10T18:13:33Z

pkg/neg/syncers/subsets_test.go

@@ -219,6 +285,39 @@ func makeNodes(startIndex, count int) []*v1.Node {
 	return nodes
 }

+func makeNodeWithNetwork(t *testing.T, name string, networksAndIPs ...string) *v1.Node {


nit: add a comment on how networksAndIPs should be populated when using this function

added, maybe we should change this to something else now that I'm looking at it?

perhaps a map?

pkg/neg/syncers/subsets_test.go

pkg/network/network.go

swetharepakula · 2023-05-10T22:58:06Z

pkg/network/network_test.go

+				ObjectMeta: metav1.ObjectMeta{Name: "test-node", Annotations: map[string]string{
+					networkv1.NorthInterfacesAnnotationKey: northInterfacesAnnotation(t, networkv1.NorthInterfacesAnnotation{
+						{
+							Network:   "default",


will the default network also be on the node like this?

no, it won't contain the default network

I changed the "default" name here not to create confusion (although it's not relevant for this test)

mmamczur · 2023-05-11T15:08:04Z

This mostly looks good to me. A few nits and cleanup comments.

Can we separate out the vendor changes into its own commit?

I've split commits (both in this PR)

swetharepakula

/approve

Only lingering comment is about cleanup in the test function to make it more understandable. I am okay for this to merge as is, and we can have followups to clean. Or if you want to address in this that is fine too.

swetharepakula · 2023-05-11T21:53:57Z

pkg/neg/syncers/subsets_test.go

@@ -219,6 +285,39 @@ func makeNodes(startIndex, count int) []*v1.Node {
 	return nodes
 }

+func makeNodeWithNetwork(t *testing.T, name string, networksAndIPs ...string) *v1.Node {


perhaps a map?

mmamczur · 2023-05-16T07:11:51Z

Only lingering comment is about cleanup in the test function to make it more understandable. I am okay for this to merge as is, and we can have followups to clean. Or if you want to address in this that is fine too.

changed it to use a map. it makes invocations a bit more verbose with all these map[string]string{} but it is easier to understand

cezarygerard · 2023-05-16T12:49:38Z

/unhold

cezarygerard · 2023-05-16T12:49:45Z

/lgtm

k8s-ci-robot · 2023-05-16T12:49:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, mmamczur, swetharepakula

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~~ [cezarygerard,swetharepakula]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 17, 2023

k8s-ci-robot requested review from aojea and prameshj March 17, 2023 16:00

mmamczur force-pushed the multi-networking-neg-controller branch from 9cae185 to 2aed49d Compare March 20, 2023 08:20

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 Mar 20, 2023

mmamczur force-pushed the multi-networking-neg-controller branch 2 times, most recently from a0458bd to b55f03a Compare March 24, 2023 09:49

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

mmamczur mentioned this pull request Mar 24, 2023

Multi networking support in the L4 ILB controller #2013

Merged

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

mmamczur force-pushed the multi-networking-neg-controller branch 2 times, most recently from b4ffb2b to 14322fc Compare April 7, 2023 12:37

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

aojea reviewed Apr 8, 2023

View reviewed changes

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

mmamczur force-pushed the multi-networking-neg-controller branch from 14322fc to 0b2af7a Compare April 12, 2023 12:52

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

mmamczur force-pushed the multi-networking-neg-controller branch from 0b2af7a to 03071ec Compare April 12, 2023 13:18

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

mmamczur force-pushed the multi-networking-neg-controller branch from 03071ec to 38abfa6 Compare April 19, 2023 10:30

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

mmamczur force-pushed the multi-networking-neg-controller branch from d6d56be to 4b5325d Compare April 25, 2023 08:01

cezarygerard reviewed Apr 27, 2023

View reviewed changes

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

mmamczur force-pushed the multi-networking-neg-controller branch from 4b5325d to 3416001 Compare April 28, 2023 13:54

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

mmamczur force-pushed the multi-networking-neg-controller branch from 3416001 to e51bfc8 Compare April 28, 2023 14:31

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

mmamczur force-pushed the multi-networking-neg-controller branch from e51bfc8 to 77366c0 Compare May 4, 2023 14:44

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

k8s-ci-robot assigned swetharepakula May 4, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2023

swetharepakula reviewed May 10, 2023

View reviewed changes

mmamczur force-pushed the multi-networking-neg-controller branch from 77366c0 to d7a438d Compare May 11, 2023 13:29

Vendor changes for multinetwork related testing sources.

02a9160

mmamczur force-pushed the multi-networking-neg-controller branch from d7a438d to 0a95e68 Compare May 11, 2023 15:07

swetharepakula reviewed May 15, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2023

Multi Network support in the NEG controller

dbc0dcb

mmamczur force-pushed the multi-networking-neg-controller branch from 0a95e68 to dbc0dcb Compare May 16, 2023 07:09

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2023

k8s-ci-robot assigned cezarygerard May 16, 2023

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

k8s-ci-robot merged commit 0dab8fd into kubernetes:master May 16, 2023

Multi networking neg controller #2023

Multi networking neg controller #2023

Conversation

mmamczur commented Mar 17, 2023

k8s-ci-robot commented Mar 17, 2023

swetharepakula commented Mar 17, 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

mmamczur Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

cezarygerard left a 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

cezarygerard Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

cezarygerard Apr 28, 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

bowei commented May 4, 2023

swetharepakula left a 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

mmamczur commented May 11, 2023

swetharepakula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmamczur commented May 16, 2023

cezarygerard commented May 16, 2023

cezarygerard commented May 16, 2023

k8s-ci-robot commented May 16, 2023

mmamczur Apr 25, 2023 •

edited

Loading

cezarygerard Apr 28, 2023 •

edited

Loading

cezarygerard Apr 28, 2023 •

edited

Loading