Add monitors+etp:local testcases #60

rlobillo · 2022-11-16T13:40:22Z

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

Two tests, one for amphora and another for ovn provider.

They create an UDP lb svc enabling openstack monitors and configuring ETP:Local and then checking that the existing lbMethod is successfully applied.

These test will be skipped for Kuryr NetworkType.

MaysaMacedo · 2022-11-24T15:05:06Z

test/extended/openstack/loadbalancers.go

+		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() {
+
+			g.By("Checking cluster configuration")
+			cloudProviderConfig, err := getConfig(oc.AdminKubeClient(),


Could this be added on the beforeeach? All the test cases seems to fetch it

Sure! I will think about moving this to BeforeEach during the rebases as soon as #58 get merged

MaysaMacedo · 2022-11-24T15:17:06Z

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(context.Background(), metav1.ListOptions{})
+			o.Expect(err).NotTo(o.HaveOccurred())
+			//lbMethod can be something different to ROUND_ROBIN as monitors && ETP:Local are enabled on this test:


do we need to check something specific around the use of ETP:local and the Pods?
It seems only the lbMethods are being checked

mmm I don't see more checks to add here.

On line 209 we are checking that the spec is there, and we are checking also that the LbMethod different to ROUND_ROBIN is applied: In D/S CI, we are enabling monitors and setting SOURCE_IP LbMethod. Under these circunstances, a unique pod should be accesed. That is checked on line 335.

That will confirm that the ETP:Local is in place.

rlobillo · 2022-11-29T16:11:42Z

/retest

rlobillo · 2022-12-08T13:15:03Z

@dulek @MaysaMacedo PTAL, this is the next pair of testcases on the queue for UDP LB epic

rlobillo · 2022-12-09T08:01:25Z

/retest

dulek

/lgtm

Minor remarks inline, feel free to ignore them if you think they're not worth it.

dulek · 2022-12-13T14:28:59Z

test/extended/openstack/loadbalancers.go

+			monitorDelay := 10
+			monitorTimeout := 20
+			monitorMaxRetries := 2


I wonder if these shouldn't be constants somewhere.

It's only used on the test so I prefer to have it defined at the top of the test.

dulek · 2022-12-13T14:29:15Z

test/extended/openstack/loadbalancers.go

+				svc.SetAnnotations(map[string]string{
+					"loadbalancer.openstack.org/enable-health-monitor":      "true",
+					"loadbalancer.openstack.org/health-monitor-delay":       fmt.Sprintf("%d", monitorDelay),
+					"loadbalancer.openstack.org/health-monitor-timeout":     fmt.Sprintf("%d", monitorTimeout),
+					"loadbalancer.openstack.org/health-monitor-max-retries": fmt.Sprintf("%d", monitorMaxRetries),
+				})


And this could probably be a helper function.

for the moment it's only used once, and also I think this approach is more readable.

dulek · 2022-12-13T14:30:59Z

test/extended/openstack/loadbalancers.go

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


This is a bit redundant, it's checked by WaitForLoadBalancer: https://github.com/openshift/kubernetes/blob/020cf2d7aa2011147a89c9bd4063c8237b71a08f/test/e2e/framework/service/jig.go#L581-L586

good catch! I'm removing it

dulek · 2022-12-13T14:34:13Z

test/extended/openstack/loadbalancers.go

+			o.Expect(monitor.AdminStateUp).Should(o.BeTrue(), "Unexpected healthmonitor adminStateUp on Openstack LoadBalancer Pool: %q", pool.Name)
+			o.Expect(monitor.Delay).Should(o.Equal(monitorDelay), "Unexpected healthmonitor delay on Openstack LoadBalancer Pool: %q", pool.Name)
+			o.Expect(monitor.Timeout).Should(o.Equal(monitorTimeout), "Unexpected healthmonitor timeout on Openstack LoadBalancer Pool: %q", pool.Name)
+			o.Expect(monitor.MaxRetries).Should(o.Equal(monitorMaxRetries), "Unexpected healthmonitor MaxRetries on Openstack LoadBalancer Pool: %q", pool.Name)


You could probably test Type as well, but that's a bit tricky as it depends on OpenStack version. Probably not worth it.

agree, I think that's internal implementatoin.

Covers partially the epic https://issues.redhat.com/browse/OSASINFRA-2753 two equivalent test, one for amphora and another for ovn provider. They create an UDP lb svc enabling openstack monitors and configuring ETP:Local and then checking that the existing lbMethod is successfully applied. These test will be skipped for Kuryr NetworkType.

MaysaMacedo · 2022-12-13T18:25:37Z

/lgtm

rlobillo · 2022-12-14T09:45:22Z

/test test

rlobillo · 2022-12-14T09:46:27Z

/test verify
/test images
/test build

dulek · 2022-12-14T16:21:59Z

/approve
/override ci/prow/test-kuryr

openshift-ci · 2022-12-14T16:24:33Z

[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 · 2022-12-14T16:24:56Z

@dulek: Overrode contexts on behalf of dulek: ci/prow/test-kuryr

In response to this:

/approve
/override ci/prow/test-kuryr

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.

rlobillo · 2022-12-14T18:20:42Z

/override ci/prow/test-kuryr

openshift-ci · 2022-12-14T18:21:26Z

@rlobillo: Overrode contexts on behalf of rlobillo: ci/prow/test-kuryr

In response to this:

/override ci/prow/test-kuryr

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 · 2022-12-14T18:21:28Z

@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.

rlobillo · 2022-12-14T19:28:30Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-12-14T19:29:09Z

@rlobillo: #60 failed to apply on top of branch "release-4.12":

Applying: Add monitors+etp:local testcases
Using index info to reconstruct a base tree...
M	test/extended/openstack/loadbalancers.go
M	test/extended/util/annotate/generated/zz_generated.annotations.go
Falling back to patching base and 3-way merge...
Auto-merging test/extended/util/annotate/generated/zz_generated.annotations.go
Auto-merging test/extended/openstack/loadbalancers.go
CONFLICT (content): Merge conflict in test/extended/openstack/loadbalancers.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add monitors+etp:local testcases
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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.

rlobillo · 2022-12-15T11:46:14Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-12-15T11:46:59Z

@rlobillo: new pull request created: #67

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 gryf and luis5tb November 16, 2022 13:40

MaysaMacedo reviewed Nov 24, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022

rlobillo force-pushed the etp_local branch from b61a2ac to 3f09830 Compare December 7, 2022 11:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2022

rlobillo force-pushed the etp_local branch from 3f09830 to a4f7315 Compare December 8, 2022 13:11

rlobillo requested review from MaysaMacedo and removed request for gryf and luis5tb December 8, 2022 13:14

dulek reviewed Dec 13, 2022

View reviewed changes

openshift-ci bot assigned dulek Dec 13, 2022

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

rlobillo force-pushed the etp_local branch from a4f7315 to e2bbf6a Compare December 13, 2022 14:56

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

openshift-ci bot assigned MaysaMacedo Dec 13, 2022

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

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

openshift-merge-robot merged commit 2ebdb67 into openshift:main Dec 14, 2022

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

[release-4.12] Add monitors+etp:local testcases #67

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add monitors+etp:local testcases #60

Add monitors+etp:local testcases #60

rlobillo commented Nov 16, 2022

MaysaMacedo Nov 24, 2022

rlobillo Nov 29, 2022

MaysaMacedo Nov 24, 2022

rlobillo Nov 29, 2022

rlobillo commented Nov 29, 2022

rlobillo commented Dec 8, 2022

rlobillo commented Dec 9, 2022

dulek left a comment

dulek Dec 13, 2022

rlobillo Dec 13, 2022

dulek Dec 13, 2022

rlobillo Dec 13, 2022

dulek Dec 13, 2022

rlobillo Dec 13, 2022

dulek Dec 13, 2022

rlobillo Dec 13, 2022

MaysaMacedo commented Dec 13, 2022

rlobillo commented Dec 14, 2022

rlobillo commented Dec 14, 2022

dulek commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

rlobillo commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

rlobillo commented Dec 14, 2022

openshift-cherrypick-robot commented Dec 14, 2022

rlobillo commented Dec 15, 2022

openshift-cherrypick-robot commented Dec 15, 2022

Add monitors+etp:local testcases #60

Add monitors+etp:local testcases #60

Conversation

rlobillo commented Nov 16, 2022

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

rlobillo commented Nov 29, 2022

rlobillo commented Dec 8, 2022

rlobillo commented Dec 9, 2022

dulek 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

MaysaMacedo commented Dec 13, 2022

rlobillo commented Dec 14, 2022

rlobillo commented Dec 14, 2022

dulek commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

rlobillo commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

rlobillo commented Dec 14, 2022

openshift-cherrypick-robot commented Dec 14, 2022

rlobillo commented Dec 15, 2022

openshift-cherrypick-robot commented Dec 15, 2022