Configure TemplateLB with all host addresses #3557

zeeke · 2023-04-28T13:36:09Z

- What this PR does and why is it needed

This PR makes NodePort services with ExternalClusterPolicy=Cluster to be served
on every host address.
It's meant to be a continuation of

Serve NodePort services on secondary IP addresses #3328

- Special notes for reviewers

cc @dceara

- How to verify it

- Description for the changelog

tssurya

main logic lgtm, some suggestions and questions

tssurya · 2023-05-02T08:06:30Z

test/e2e/e2e.go

@@ -2276,18 +2276,26 @@ var _ = ginkgo.Describe("e2e ingress traffic validation", func() {
 		// This test verifies a NodePort service is reachable on manually added IP addresses.
 		ginkgo.It("for NodePort services", func() {


e2e tests for services belong in services.go file (I know we have some older tests lying around in e2e.go but goal is to have newer tests in the specific file created for services tests)

Moved the test to service.go

tssurya · 2023-05-02T08:10:40Z

go-controller/pkg/ovn/controller/services/lb_config.go

may I ask why the tests in the first PR didn't catch this? I know @dceara mentioned something to me but not sure I got the full picture...basically how was the logic being done in the first PR versus this one? If we hadn't added templating for multiple nodeIP's how was it passing in the first PR?

The test added in #3328 checks services with ETP=local, that AFAIU is implemented without Templates.

tssurya · 2023-05-02T08:15:57Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

-func makeLBNodeIPTemplateName(family corev1.IPFamily) string {
-	return fmt.Sprintf("%s_%v", LBVipNodeTemplate, family)
+func makeLBNodeIPTemplateNamePrefix(family corev1.IPFamily) string {
+	return fmt.Sprintf("%s_%v_", LBVipNodeTemplate, family)
 }

 // isLBNodeIPTemplateName returns true if 'name' is the node IP template name
 // for any IP family.
 func isLBNodeIPTemplateName(name string) bool {


we should change this to isLBNodeIPTemplateNamePrefix ? and update the function description as well to say we now search for prefix

Updated the description. I left the function name as is, as it doesn't check if the input string is a prefix. It returns true if the passed name is the name of a NodeIP template. Does it sound good?

tssurya · 2023-05-02T08:42:58Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

@@ -180,15 +175,15 @@ func svcCreateOrUpdateTemplateVar(nbClient libovsdbclient.Client, templateVars [
 }

 // makeLBNodeIPTemplateName creates a template name for the node IP (per family)
-func makeLBNodeIPTemplateName(family corev1.IPFamily) string {
-	return fmt.Sprintf("%s_%v", LBVipNodeTemplate, family)
+func makeLBNodeIPTemplateNamePrefix(family corev1.IPFamily) string {


nit: update the func description to say we are creating a template prefix name for the nodeIP

tssurya · 2023-05-02T09:04:10Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

+type NodeIPsTemplates struct {
+	ipFamily           corev1.IPFamily
+	templateValues     []*chassisTemplateValue
+	chassisIDtoIPCount map[string]int


Is this tracking the number of nodeIPs per chassis?

yes, going to rename it

tssurya · 2023-05-02T09:05:14Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

+type chassisTemplateValue struct {
+	templateName string
+	chassisID    string
+	value        string


what is this value here within the chassisTemplateValue ? Please add some doc strings for this one, et.too.many.value usages....

Renamed and documented. Let me know if it's clear

tssurya · 2023-05-02T09:06:36Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

@@ -215,3 +210,72 @@ func getTemplatesFromRulesTargets(rules []LBRule) TemplateMap {
 	}
 	return templates
 }
+
+// NodeIPTemplates maintains templates variables for many IP addresses per node,
+// creating template variables in the for NODEIP_IPv4_0, NODEIP_IPv4_1, NODEIP_IPv4_2, ...


Suggested change

// creating template variables in the for NODEIP_IPv4_0, NODEIP_IPv4_1, NODEIP_IPv4_2, ...

// creating template variables for NODEIP_IPv4_0, NODEIP_IPv4_1, NODEIP_IPv4_2, ...

?

tssurya · 2023-05-02T09:14:30Z

go-controller/pkg/ovn/controller/services/services_controller.go

-				c.nodeIPv4Template.Value[node.chassisID] = ipv4.String()
+			ips, err := util.MatchIPFamily(false, nodeInfo.hostAddresses)
+			if err != nil {
+				klog.Warningf("Error while searching for IPv4 host addresses: %v", err)


we should probably include the nodeInfo.name in the warning message to make it easier to map it to the node

tssurya · 2023-05-02T09:14:48Z

go-controller/pkg/ovn/controller/services/services_controller.go

-				c.nodeIPv6Template.Value[node.chassisID] = ipv6.String()
+			ips, err := util.MatchIPFamily(true, nodeInfo.hostAddresses)
+			if err != nil {
+				klog.Warningf("Error while searching for IPv6 host addresses: %v", err)


ditto: include nodeName in warning message

tssurya · 2023-05-02T09:24:29Z

go-controller/pkg/ovn/controller/services/services_controller_test.go

@@ -510,6 +510,120 @@ func TestSyncServices(t *testing.T) {
 	}
 }

+func Test_ETPCluster_NodePort_Service_WithMultipleIPAddresses(t *testing.T) {


thank you for the tests++ nit: suggestion to swap some into IPV6 addresses to cover those bits as well but I won't hold the PR for that.

I added a commit to convert that test to dualstack:
8a973fa

Test code becomes a little bit more complicated. WDYT?

I am good with this thanks @zeeke !

go-controller/pkg/ovn/controller/services/lb_config.go

coveralls · 2023-05-05T20:30:55Z

Coverage: 53.145% (+0.09%) from 53.05% when pulling ed9a280 on zeeke:node-port-svc-on-secondary-ip-rework into 7f008bd on ovn-org:master.

go-controller/pkg/ovn/controller/services/svc_template_var.go

dceara · 2023-05-08T20:47:53Z

go-controller/pkg/ovn/controller/services/svc_template_var_test.go

@@ -0,0 +1,79 @@
+package services


Thanks for adding tests!

test/e2e/e2e.go

tssurya · 2023-05-09T06:51:27Z

go-controller/pkg/ovn/controller/services/svc_template_var.go

+// AddIP adds a template variable for the specified chassis and ip address.
+func (n *NodeIPsTemplates) AddIP(chassisID string, ip net.IP) {
+
+	var i int = -1


hmm I see this part of code also changed since I previously did a review, not a fan of the starting with i = -1 just using that variable to get the total number of templates we have for the node a.k.a the nodeIPs..., if like @dceara mentions we can leverage the length and figure that out, let's do that...seems cleaner?

dceara

LGTM, thanks!

jcaamano · 2023-05-10T14:36:37Z

test/e2e/service.go

+				framework.Failf("failed to add new Addresses to node %s: %v", node.Name, err)
+			}
+
+			defer runCommand(containerRuntime, "exec", node.Name, "ip", "addr", "delete", newIP+"/32", "dev", "breth0")


nit: fail the test id this fails

tssurya

/lgtm
thanks @zeeke

TemplateLB variables for node IP addresses have to support multiple IP noda address, hence the variables are in the format of NODE_IPv4_0, NODE_IPv4_1, NODE_IPv4_2, ... Struct `NodeIPsTemplates` manage the template variables for multiple nodes that may have different number of IP addresses each. Add unit and e2e tests on TemplateLBs with multiple IP addresses. Refs: ovn-org#3328 Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

Make `Test_ETPCluster_NodePort_Service_WithMultipleIPAddresses` use v4+v6 IP addresses. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

The test about NodePort services that are reachable on every host IP address should reside in the Service suite. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

trozet · 2023-05-15T17:11:49Z

Flake:
[Fail] Load Balancer Service Tests with MetalLB [It] Should ensure load balancer service works with pmtu
/home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/service.go:885

[Fail] Load Balancer Service Tests with MetalLB [It] Should ensure load balancer service works with pmtu

https://github.com/ovn-org/ovn-kubernetes/actions/runs/4981960698/jobs/8917595516?pr=3557

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch 5 times, most recently from 7d8ff81 to 640496b Compare April 29, 2023 10:49

trozet requested a review from dceara May 1, 2023 14:45

tssurya requested changes May 2, 2023

View reviewed changes

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from 640496b to 92da78f Compare May 5, 2023 11:55

zeeke requested review from trozet, dcbw, girishmg and jcaamano as code owners May 5, 2023 11:55

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from 92da78f to 3585b2f Compare May 5, 2023 12:22

dceara reviewed May 5, 2023

View reviewed changes

go-controller/pkg/ovn/controller/services/lb_config.go Show resolved Hide resolved

go-controller/pkg/ovn/controller/services/lb_config.go Show resolved Hide resolved

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch 3 times, most recently from 99a1050 to 3626614 Compare May 5, 2023 18:05

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from 3626614 to ed9a280 Compare May 8, 2023 07:44

dceara reviewed May 8, 2023

View reviewed changes

tssurya requested changes May 9, 2023

View reviewed changes

test/e2e/e2e.go Outdated Show resolved Hide resolved

tssurya requested changes May 9, 2023

View reviewed changes

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from ed9a280 to a1accc1 Compare May 9, 2023 09:40

dceara previously approved these changes May 10, 2023

View reviewed changes

jcaamano reviewed May 10, 2023

View reviewed changes

zeeke dismissed dceara’s stale review via a385454 May 10, 2023 15:00

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch 2 times, most recently from a385454 to d6b0d8d Compare May 10, 2023 15:01

tssurya previously approved these changes May 10, 2023

View reviewed changes

zeeke dismissed tssurya’s stale review via 16d5df1 May 11, 2023 06:57

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from d6b0d8d to 16d5df1 Compare May 11, 2023 06:57

zeeke added 3 commits May 11, 2023 10:29

Dualstack Service unit test

f041b03

Make `Test_ETPCluster_NodePort_Service_WithMultipleIPAddresses` use v4+v6 IP addresses. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

e2e: Move NodePort test to Services suite

454119c

The test about NodePort services that are reachable on every host IP address should reside in the Service suite. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

zeeke force-pushed the node-port-svc-on-secondary-ip-rework branch from 16d5df1 to 454119c Compare May 11, 2023 08:29

Merge branch 'master' into node-port-svc-on-secondary-ip-rework

63f398b

trozet merged commit 723a5d7 into ovn-org:master May 15, 2023
22 checks passed

zeeke mentioned this pull request Jun 5, 2023

SDN-3555,SDN-3790,OCPBUGS-4485,OCPBUGS-14449,OCPBUGS-13863,OCPBUGS-14286,OCPBUGS-12800: Downstream Merge 1st June openshift/ovn-kubernetes#1692

Merged

3 tasks

wizhaoredhat mentioned this pull request Jun 28, 2023

Use NodePrimaryIp to set host-addresses in DPU mode #3706

Merged

zeeke mentioned this pull request Sep 7, 2023

OCPBUGS-18695: [release-4.13] Serve NodePort services on secondary IP addresses openshift/ovn-kubernetes#1860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure TemplateLB with all host addresses #3557

Configure TemplateLB with all host addresses #3557

zeeke commented Apr 28, 2023

tssurya left a comment

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 2, 2023

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 2, 2023

tssurya May 2, 2023

tssurya May 2, 2023

tssurya May 2, 2023

zeeke May 5, 2023

tssurya May 9, 2023

coveralls commented May 5, 2023 •

edited

dceara May 8, 2023

tssurya May 9, 2023

dceara left a comment

jcaamano May 10, 2023

tssurya left a comment

trozet commented May 15, 2023

		@@ -2276,18 +2276,26 @@ var _ = ginkgo.Describe("e2e ingress traffic validation", func() {
		// This test verifies a NodePort service is reachable on manually added IP addresses.
		ginkgo.It("for NodePort services", func() {

	// creating template variables in the for NODEIP_IPv4_0, NODEIP_IPv4_1, NODEIP_IPv4_2, ...
	// creating template variables for NODEIP_IPv4_0, NODEIP_IPv4_1, NODEIP_IPv4_2, ...

Configure TemplateLB with all host addresses #3557

Configure TemplateLB with all host addresses #3557

Conversation

zeeke commented Apr 28, 2023

tssurya 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

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

coveralls commented May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dceara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssurya left a comment

Choose a reason for hiding this comment

trozet commented May 15, 2023

coveralls commented May 5, 2023 •

edited