Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add udp-lb testcase for amphora provider #56

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

rlobillo
Copy link
Contributor

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

This test creates an UDP lb on Openshift 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 Amphora.

@rlobillo
Copy link
Contributor Author

/retest

@rlobillo
Copy link
Contributor Author

/retest

2 similar comments
@rlobillo
Copy link
Contributor Author

/retest

@rlobillo
Copy link
Contributor Author

/retest

@rlobillo
Copy link
Contributor Author

/retest

@dulek
Copy link
Contributor

dulek commented Oct 20, 2022

/retest

Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of comments inline, in general this makes perfect sense to me.

// Check if the provided lbMethod is applied considering the
// result map (<pod name>: <number of accesses>) and the list of pods provided.
// TO DO: Only 'ROUND_ROBIN' method implement for the moment.
func isLbMethodApplied(lbMethod string, results map[string]int, pods *v1.PodList) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't exactly see this testing other algorithms. And it feels like something that may be pretty flaky, even if we have this safeguard of 20. It's probably necessary to consult Octavia engineers on how reliable the algorithm choice is. I would consider just checking if pool was created with a correct setting instead.

Copy link
Contributor Author

@rlobillo rlobillo Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to validate the end to end functionality here... I think that checking the pool can be tricky if something change on openstack. Our plan is to run this on D/S in all the supported OSP releases so I think it is more stable this high level check than checking the pool.

About other algorithms, I think the most probable algorithm to include here is SOURCE_IP_PORT on OVN, but we agreed that monitors are needed and it is currently not supported in OVN provider. I will mention that in separate PR where I will include the test for OVN provider.

test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
o.Expect(pool.LBMethod).Should(o.Equal(lbMethod), "Unexpected LBMethod on Openstack Pool: %q", pool.LBMethod)

g.By("accessing the service 100 times from outside and storing the name of the pods answering")
time.Sleep(10 * time.Minute) // Give time to the cluster to be ready
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's pretty long time. What are we actually waiting for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still struggling with vexxhost to get the results from the pods. It is timing out sometimes. I added this long sleep (10 minutes) and the last gate run passed, so I am updating the check to allow timeouts in the queries, and reducing this timer just to see if there is no need to wait so long.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's see. Obviously a wait for some condition instead of Sleep is better, so we might want to consider what we're waiting for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally there is no need to wait anything. Just accepting some timeouts in the 100 requests is enough to provide stability on the test.

The testing for this patch is done!

test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
@rlobillo
Copy link
Contributor Author

/retest

1 similar comment
@rlobillo
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2022

@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
Copy link
Contributor Author

@dulek please help to get this merged :) I'd like to include more tests based on this one.

@dulek
Copy link
Contributor

dulek commented Oct 24, 2022

/lgtm

Looks good now, but I'd rather ask either @MaysaMacedo or @pierreprinetti to give final /approve.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022
Copy link
Contributor

@MaysaMacedo MaysaMacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major requests, just two questions that can be answered later.
/approve

o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(pool.Protocol).Should(o.Equal("UDP"), "Unexpected protocol on Openstack LoadBalancer Pool: %q", pool.Name)
//Set as OFFLINE in vexxhost despite the lb is operative
//o.Expect(pool.OperatingStatus).Should(o.Equal("ONLINE"), "Unexpected Operating Status on Openstack Pool: %q", pool.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was a ticket open about this to vexxhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, I did not

results[podName]++
}
}
e2e.Logf("Pods accessed after 100 UDP requests:\n%v\n", results)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logs are only appearing if you run the test isolated:

KUBECONFIG=~/.kube/config ./openstack-tests run-test "[sig-installer][Suite:openshift/openstack][lb] The Openstack platform should create an UDP Amphora LoadBalancer when an UDP svc with type:LoadBalancer is created on Openshift [Suite:openshift/conformance/parallel]"
or if you run the whole testsuite but the test fails.

@MaysaMacedo
Copy link
Contributor

Great work!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaysaMacedo

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4b97e38 into openshift:main Oct 26, 2022
@rlobillo rlobillo deleted the udplb branch March 14, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants