Add tcp lb testcases #106

rlobillo · 2023-06-01T15:13:12Z

Covering tech debt in openstack-test

https://issues.redhat.com/browse/OSASINFRA-2412

rlobillo · 2023-06-02T11:15:48Z

/cc @dulek Could you please review this?

rlobillo · 2023-06-02T14:53:53Z

/cc @dulek

dulek · 2023-06-02T15:03:14Z

test/extended/openstack/loadbalancers.go

-		// https://issues.redhat.com/browse/OSASINFRA-2753
-		g.It(fmt.Sprintf("should create an UDP %s LoadBalancer when an UDP svc with type:LoadBalancer is created on Openshift", lbProviderUnderTest), func() {
+		for _, j := range availableProtocolsUnderTests {
+			protocolUnderTest := j


Why like this? Just assign it directly to protocolUnderTest:

for _, protocolUnderTest := range availableProtocolsUnderTests {

That's something we observed in the previous round of tests. Honestly, I don't know why but the whole engine that is calculating the list of tests don't work if I don't do that. I copied the current approach from origin, so I assume they already hit that and that was their solution.

right, goroutines in for cycles...

To make it a bit more digestible, try this:

for i := range availableLbProvidersUnderTests { lbProviderUnderTest := availableLbProvidersUnderTests[i] for j := range availableProtocolsUnderTests { protocolUnderTest := availableProtocolsUnderTests[j]

I’d also check if this Ginkgo thing likes being called in a loop.

Okay, fair enough, origin's approach works for me.

dulek · 2023-06-02T15:04:46Z

test/extended/openstack/loadbalancers.go

 // Error if there is no answer or cannot access the port after 5 seconds.
-func getPodNameThroughUdpLb(ip string, port string, message string) (string, error) {
+func getPodNameThroughTcpLb(ip string, port string, message string) (string, error) {


This seems unused?

oh right! thanks for noticing

dulek · 2023-06-02T15:06:09Z

test/extended/openstack/loadbalancers.go

-		// https://issues.redhat.com/browse/OCPBUGS-2350
-		g.It(fmt.Sprintf("should apply lb-method on UDP %s LoadBalancer when an UDP svc with monitors and ETP:Local is created on Openshift", lbProviderUnderTest), func() {
+		for _, j := range availableProtocolsUnderTests {
+			protocolUnderTest := j


for j := range availableProtocolsUnderTests { protocolUnderTest := availableProtocolsUnderTests[j]

dulek · 2023-06-02T15:06:30Z

Thanks for doing this Ramon!

rlobillo · 2023-06-14T10:42:17Z

/cc @pierreprinetti

pierreprinetti · 2023-06-14T11:52:08Z

test/extended/openstack/loadbalancers.go

-			skipIfNotLbProvider(lbProviderUnderTest, cloudProviderConfig)
+			// https://issues.redhat.com/browse/OSASINFRA-2753
+			// https://issues.redhat.com/browse/OSASINFRA-2412
+			g.It(fmt.Sprintf("should create an %s %s LoadBalancer when an %s svc with type:LoadBalancer is created on Openshift", protocolUnderTest, lbProviderUnderTest, protocolUnderTest), func() {


nit: s/when an %s svc/when a %s svc/, which works for both UDP and TCP

pierreprinetti · 2023-06-14T12:09:04Z

test/extended/openstack/loadbalancers.go

-// Send message on provided ip and UDP port and return the answer.
-// Error if there is no answer or cannot access the port after 5 seconds.
-func getPodNameThroughUdpLb(ip string, port string, message string) (string, error) {
+func getPodNameThroughLb(ip string, port string, protocol v1.Protocol, message string) (string, error) {


While you're at it, please accept port as int and cast within this function (for example, with strconv.Itoa())

I'd prefer not do so as the port is int32 and I'm passing it converted to a string. I don't want to add another cast to convert int32 to int and then int to string. I'd let it be as it is now.

why is it int32?

ah ServicePort. Well it can be int32 in this function signature too

in such case I cannot use strconv.Itoa(port) or equivalent strconv function, at least I wasn't able to find it.

strconv.FormatInt(int64(i), 10)

pierreprinetti · 2023-06-14T12:19:48Z

test/extended/openstack/loadbalancers.go

+
+		// receive message from server
+		buffer := make([]byte, 1024)
+		n, _, err := conn.ReadFromUDP(buffer)


Since you're discarding the UPDAddr, this could be conn.ReadFrom(buffer)

pierreprinetti · 2023-06-14T12:34:28Z

test/extended/openstack/loadbalancers.go

+		}
+		defer resp.Body.Close()
+
+		body, err := ioutil.ReadAll(resp.Body)


nit: use io.ReadAll

pierreprinetti · 2023-06-14T12:51:08Z

test/extended/openstack/loadbalancers.go

@@ -674,6 +719,9 @@ func createTestDeployment(depName string, labels map[string]string, replicas int
 	isFalse := false
 	isTrue := true
 	netExecParams := "--" + strings.ToLower(string(protocol)) + "-port=" + fmt.Sprintf("%d", port)
+	if protocol == v1.ProtocolTCP {


nit: to improve readability, you could treat all cases at the same time (e.g. with a case/switch) instead of setting a default and then override for TCP.

Something like:

var netExecParams string switch protocol { case v1.ProtocolTCP: netExecParams = fmt.Sprintf("--http-port=%d", port) default: netExecParams = fmt.Sprintf("--%s-port=%d", strings.ToLower(string(protocol)), port) }

<3 it. Thank you.

pierreprinetti

I'm far from being a LB expert, so this PR is probably not for me, sorry. I just added a couple comments on the form, that you can totally ignore. For what it's worth, LGTM!

Covering tech debt in openstack-test https://issues.redhat.com/browse/OSASINFRA-2412

rlobillo · 2023-06-19T14:01:04Z

I'm far from being a LB expert, so this PR is probably not for me, sorry. I just added a couple comments on the form, that you can totally ignore. For what it's worth, LGTM!

Thanks for your inputs!

MaysaMacedo · 2023-06-19T14:03:40Z

/cc

openshift-ci · 2023-06-19T15:35:54Z

@rlobillo: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

dulek · 2023-06-26T16:52:29Z

test/extended/openstack/loadbalancers.go

-		// https://issues.redhat.com/browse/OSASINFRA-2753
-		g.It(fmt.Sprintf("should create an UDP %s LoadBalancer when an UDP svc with type:LoadBalancer is created on Openshift", lbProviderUnderTest), func() {
+		for _, j := range availableProtocolsUnderTests {
+			protocolUnderTest := j


Okay, fair enough, origin's approach works for me.

dulek · 2023-06-26T17:02:13Z

/approve

I'll leave LGTM for @MaysaMacedo.

openshift-ci · 2023-06-26T17:05:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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~~ [dulek]

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

openshift-ci · 2023-06-26T17:05:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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~~ [dulek]

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

dulek · 2023-06-26T17:09:57Z

Whoops, my mistake, I didn't think that it'll catch that LGTM. Any review Maysa will make will need to go through a follow up patch.

MaysaMacedo · 2023-06-26T22:35:16Z

test/extended/openstack/loadbalancers.go

-			e2e.Logf("Pods accessed after 100 UDP requests:\n%v\n", results)
-			pods, err := oc.KubeClient().CoreV1().Pods(oc.Namespace()).List(ctx, metav1.ListOptions{})
-			o.Expect(err).NotTo(o.HaveOccurred())
+				e2e.Logf("Pods accessed after 100 UDP requests:\n%v\n", results)


looks like the mention to UDP is a leftover here

Thanks for noticing @MaysaMacedo
I created this PR to address this: #112

hey that's a wonderful opportunity for turning port into a number!

MaysaMacedo

LGTM. I just noticed a leftover

openshift-ci bot requested review from eurijon and stephenfin June 1, 2023 15:21

openshift-ci bot requested a review from dulek June 2, 2023 14:53

dulek suggested changes Jun 2, 2023

View reviewed changes

openshift-ci bot assigned dulek Jun 2, 2023

rlobillo force-pushed the tcp-lb branch from 69d4a44 to 2a7ab35 Compare June 2, 2023 15:12

rlobillo requested a review from dulek June 13, 2023 07:31

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2023

rlobillo force-pushed the tcp-lb branch from 2a7ab35 to e2bc78c Compare June 14, 2023 10:39

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2023

openshift-ci bot requested a review from pierreprinetti June 14, 2023 10:42

rlobillo force-pushed the tcp-lb branch from e2bc78c to aa1afb0 Compare June 14, 2023 11:03

pierreprinetti reviewed Jun 14, 2023

View reviewed changes

Add tcp lb testcase

7701e77

Covering tech debt in openstack-test https://issues.redhat.com/browse/OSASINFRA-2412

rlobillo force-pushed the tcp-lb branch from aa1afb0 to 7701e77 Compare June 19, 2023 14:00

openshift-ci bot requested a review from MaysaMacedo June 19, 2023 14:03

dulek approved these changes Jun 26, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2023

openshift-merge-robot merged commit 88a4f2b into openshift:main Jun 26, 2023
6 checks passed

MaysaMacedo reviewed Jun 26, 2023

View reviewed changes

pierreprinetti deleted the tcp-lb branch June 27, 2023 10:11

Add tcp lb testcases #106

Add tcp lb testcases #106

Conversation

rlobillo commented Jun 1, 2023

rlobillo commented Jun 2, 2023

rlobillo commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Jun 14, 2023 • edited

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

dulek commented Jun 2, 2023

rlobillo commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Jun 14, 2023 • edited

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

pierreprinetti left a comment • edited

Choose a reason for hiding this comment

rlobillo commented Jun 19, 2023

MaysaMacedo commented Jun 19, 2023

openshift-ci bot commented Jun 19, 2023

Choose a reason for hiding this comment

dulek commented Jun 26, 2023

openshift-ci bot commented Jun 26, 2023

openshift-ci bot commented Jun 26, 2023

dulek commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaysaMacedo left a comment

Choose a reason for hiding this comment

pierreprinetti Jun 14, 2023 •

edited

pierreprinetti Jun 14, 2023 •

edited

pierreprinetti left a comment •

edited