Add udp-lb testcase for ovn provider #58

rlobillo · 2022-11-04T15:50:57Z

Covers partially the epic https://issues.redhat.com/browse/OSASINFRA-2753

This test creates an UDP lb when OVN provider is configured on the cluster and checks the created Openstack resources. It also sends 100 requests to the LoadBalancer Service and analyze the pods' responses so that the e2e functionality is verified.

This test will be skipped for Kuryr NetworkType and if cloudProviderConfig is configuring a different lb-provider than OVN.

rlobillo · 2022-11-04T20:07:04Z

/retest

rlobillo · 2022-11-07T14:14:55Z

/retest

rlobillo · 2022-11-08T16:40:38Z

/retest

rlobillo · 2022-11-08T22:23:40Z

/retest

MaysaMacedo · 2022-11-11T15:39:45Z

do we expect the configuration to be manually updated before running the test?
Or should the test update the configuration to use the OVN driver?

rlobillo · 2022-11-14T09:46:57Z

Hello @MaysaMacedo

Yes, we expect the configuration to be manually updated before running the test. The configuration change takes around 30-40 minutes and it can affect the run of the rest of the tests.

In D/S CI we are doing the cloud-provider-config changes and running the openstack-tests sequentially: https://code.engineering.redhat.com/gerrit/gitweb?p=rhos-qe-jenkins.git;hb=refs%2Fchanges%2F16%2F434216%2F4;f=jobs%2Fdefaults%2Fstages%2Fir_openshift_udplb_tests_run.groovy.inc

rlobillo · 2022-11-18T09:16:22Z

/retest

MaysaMacedo · 2022-11-18T11:03:46Z

/retest

MaysaMacedo · 2022-11-18T13:53:16Z

/retest

MaysaMacedo · 2022-11-18T16:58:09Z

I opened a ticket to vexxhost about the issue seen here

MaysaMacedo · 2022-11-21T08:54:06Z

/retest

MaysaMacedo · 2022-11-21T18:23:02Z

/test test

MaysaMacedo

It looks good! There are just some minor remarks:

MaysaMacedo · 2022-11-24T13:32:15Z

test/extended/openstack/loadbalancers.go

-		o.Expect(err).NotTo(o.HaveOccurred())
-		o.Expect(fip.FloatingIP).Should(o.Equal(svcIp), "Unexpected floatingIp in the Openstack LoadBalancer")
-		o.Expect(lb.Pools).Should(o.HaveLen(1), "Unexpected number of pools on Openstack LoadBalancer %q", lb.Name)
+	for _, i := range availableLbProvidersUnderTests {


instead of i, could it be lbProviderUnderTest so we avoid the following line?

I don't know why but your proposal did not work. I tried that, but if I don't set the variable later, the loop is not working as expected. I found this approach on origin repo, for example: https://github.com/openshift/origin/blob/master/test/extended/apiserver/apply.go#L57

Instead of looping like this shouldn't we just check the LB provider that's configured and then when LB is created in Octavia checking if it has correct provider?

Hi @dulek

The intention is to have separated tests per provider so we can track if the feature is working with Amphora and/or OVN provider at a glance.

That's very useful for our reporting tool (ReportPortal) and conceptually I think they're different tests because they're testing different things. What do you think?

They're testing different providers really. But I can't argue with the concept of a separate test if you need it like this for your tools. The loop and skip just feels a bit weird here.

Do you need both tests to appear in every result set, even if skipped? If so, I guess for loop is the way to go. Otherwise you could check LB provider before g.It(fmt.Sprintf("should create an UDP %s LoadBalancer when an UDP svc with type:LoadBalancer is created on Openshift", lbProviderUnderTest).

Thanks! Done

MaysaMacedo · 2022-11-24T13:34:04Z

test/extended/openstack/loadbalancers.go

+			lbProvider, err := getClusterLoadBalancerSetting("lb-provider", cloudProviderConfig)
+			o.Expect(err).NotTo(o.HaveOccurred())
+			if lbProvider != strings.ToLower(lbProviderUnderTest) {
+				e2eskipper.Skipf("Test not applicable for LoadBalancer provider different than %s: %q", lbProviderUnderTest, lbProvider)


might be good to add "cluster is configured with:"

MaysaMacedo · 2022-11-24T13:38:20Z

test/extended/openstack/loadbalancers.go

+			labels := map[string]string{"app": "udp-lb-default-dep"}
+			depName := "udp-lb-default-dep"
+			svcName := "udp-lb-default-svc"
+			testDeployment := e2edeployment.NewDeployment(depName, 2, labels, "udp-test",


it would be good to add a step of g.By("Creating deployment")

MaysaMacedo · 2022-11-24T13:39:13Z

test/extended/openstack/loadbalancers.go

+
+			jig := e2eservice.NewTestJig(clientSet, oc.Namespace(), svcName)
+			jig.Labels = labels
+			svc, err := jig.CreateLoadBalancerService(5*time.Minute, func(svc *v1.Service) {


it would be good to add a step of g.By("Creating Load-Balancer")

MaysaMacedo · 2022-11-24T13:40:51Z

test/extended/openstack/loadbalancers.go

+			g.By("Checks from openshift perspective")
+			o.Expect(svc.GetAnnotations()["loadbalancer.openstack.org/load-balancer-id"]).ShouldNot(o.BeEmpty(),
+				"load-balancer-id annotation missing")
+			loadBalancerId := svc.GetAnnotations()["loadbalancer.openstack.org/load-balancer-id"]


can this be defined before line 103 and then use the variable on the expect check?

I need to know if the key is present before assigning it, right?

You'll get empty string if it's not assigned, so it's safe to just save it into the var and then do the checks: https://go.dev/play/p/hP_VtvL_K8t

Thanks! Done

MaysaMacedo · 2022-11-24T13:43:08Z

test/extended/openstack/loadbalancers.go

+			o.Expect(err).NotTo(o.HaveOccurred())
+			o.Expect(lb.Provider).Should(o.Equal(strings.ToLower(lbProviderUnderTest)), "Unexpected provider in the Openstack LoadBalancer")
+
+			svcIp := svc.Status.LoadBalancer.Ingress[0].IP


ditto before line 106

I need to know if the key is present before assigning it, right?

Well, the string will be empty in worst case so no problem there, but you should check for length of the Ingress array, otherwise if it's empty, you'll end up with some golang panic here.

MaysaMacedo · 2022-11-30T09:14:31Z

/retest
/lgtm
/hold for CI result

rlobillo · 2022-11-30T16:59:52Z

/retest

openshift-ci · 2022-11-30T19:46: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.

MaysaMacedo · 2022-12-01T08:58:50Z

/hold cancel
I will leave the approve for someone else. Maybe @dulek?

dulek · 2022-12-01T09:42:18Z

/approve

openshift-ci · 2022-12-01T09:42:41Z

[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

rlobillo · 2022-12-02T10:53:05Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-12-02T10:53:47Z

@rlobillo: new pull request created: #62

In response to this:

/cherry-pick release-4.12

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.

openshift-ci bot requested review from itzikb-redhat and luis5tb November 4, 2022 15:52

rlobillo mentioned this pull request Nov 4, 2022

UDP-test: Fix lbMethod comparison #57

Closed

rlobillo force-pushed the udplb_ovn branch 2 times, most recently from 20af315 to 04c2b6d Compare November 16, 2022 13:02

MaysaMacedo reviewed Nov 24, 2022

View reviewed changes

rlobillo force-pushed the udplb_ovn branch from 04c2b6d to a1250d5 Compare November 29, 2022 14:54

rlobillo mentioned this pull request Nov 29, 2022

Add monitors+etp:local testcases #60

Merged

Add udp-lb testcase for ovn provider

8fb4f84

rlobillo force-pushed the udplb_ovn branch from a1250d5 to 8fb4f84 Compare November 29, 2022 17:19

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022

openshift-ci bot assigned MaysaMacedo Nov 30, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2022

openshift-merge-robot merged commit d8d1422 into openshift:main Dec 1, 2022

openshift-cherrypick-robot mentioned this pull request Dec 2, 2022

[release-4.12] Add udp-lb testcase for ovn provider #62

Merged

rlobillo deleted the udplb_ovn branch March 14, 2023 16:34

Add udp-lb testcase for ovn provider #58

Add udp-lb testcase for ovn provider #58

Conversation

rlobillo commented Nov 4, 2022

rlobillo commented Nov 4, 2022

rlobillo commented Nov 7, 2022

rlobillo commented Nov 8, 2022

rlobillo commented Nov 8, 2022

MaysaMacedo commented Nov 11, 2022

rlobillo commented Nov 14, 2022

rlobillo commented Nov 18, 2022

MaysaMacedo commented Nov 18, 2022

MaysaMacedo commented Nov 18, 2022

MaysaMacedo commented Nov 18, 2022

MaysaMacedo commented Nov 21, 2022

MaysaMacedo commented Nov 21, 2022

MaysaMacedo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaysaMacedo commented Nov 30, 2022

rlobillo commented Nov 30, 2022

openshift-ci bot commented Nov 30, 2022

MaysaMacedo commented Dec 1, 2022

dulek commented Dec 1, 2022

openshift-ci bot commented Dec 1, 2022

rlobillo commented Dec 2, 2022

openshift-cherrypick-robot commented Dec 2, 2022