E2E refactor podcreation #149

mangelajo · 2019-09-12T08:44:49Z

This is ready for review, but please look only at the 2nd patch: 6168d2b

The concept of TestPodConfig and TestPod is defined, letting us create
testing pods from a single interface, and automating the creation
of UUID data to exchange.

As late modifications has shown that the number of parameters keeps growing
having a single fixture to create test pods will make easier to write
and read E2E tests.

Depends-On: #148

sridhargaddam · 2019-09-15T17:43:18Z

test/e2e/dataplane/tcp_pod_to_service.go


-func testPod2ServiceTCP(f *framework.Framework) {
+	It("Should be able to perform a Pod to Service TCP connection and exchange data between different clusters NonGW to GW", func() {


IMO, the following two tests should cover most of the use-cases.

GatewayNode to GatewayNode

nonGWNode to nonGWNode

The second use-case is like a superset and it covers most of the combinations.

So, please change this test (line 17) to Gateway to Gateway.

Also, make this as the first test to run and the above one (i.e., line 13) as the second test.

test/e2e/framework/network_pods.go

The concept of TestPodConfig and TestPod is defined, letting us create testing pods from a single interface, and automating the creation of UUID data to exchange. As late modifications has shown that the number of parameters keeps growing having a single fixture to create test pods will make easier to write and read E2E tests. Depends-On: submariner-io#148

mpeterson

LGTM

test/e2e/framework/network_pods.go

mpeterson

Oh, I missed that you have a merge commit. Please rebase to avoid it. There is no need to have merge commits as part of the PR.

mangelajo · 2019-09-23T11:46:14Z

@mpeterson that's the aautomatic "update" by github. I can merge it later as a squash and I think it goes away.

mangelajo · 2019-10-27T07:27:10Z

@mpeterson merge commits aren’t an issue, with the squash and merge github strategy it will result on a single commit on top of the existing master .

mkolesnik · 2019-10-29T10:09:17Z

test/e2e/dataplane/tcp_pod_to_service.go

-	Expect(exitStatusC).To(Equal(int32(0)))
+	connectorPod.WaitForFinishStatus()
+	framework.Logf("Connector output\n%s", keepLines(connectorPod.TerminationMessage, 2))
+	Expect(connectorPod.TerminationCode).To(Equal(int32(0)))


This should be in a different method (or two)..

createListenerConnectorPair should be aboult the creation of these pods not verifying they actually did something

It doesn't seem we would ever want to call create w/o verify. We're going to also want to test pod -> pod in addition to pod -> service. How about we call this runAndVerifyNetworkPod2ServicePair and then add runAndVerifyNetworkPod2PodPair later?

mkolesnik · 2019-10-29T10:47:34Z

test/e2e/framework/network_pods.go

 // create a test pod inside the current test namespace on the specified cluster.
 // The pod will listen on TestPort over TCP, send sendString over the connection,
 // and write the network response in the pod  termination log, then exit with 0 status
-func (f *Framework) CreateTCPCheckListenerPod(cluster ClusterIndex, scheduling TestPodScheduling, sendString string) *v1.Pod {
+func (tp *TestPod) buildTCPCheckListenerPod() {


Since you're already OOP'ing it, this function is super similar to buildTCPCheckConnectorPod.. Why not have one function that gets the necessary parameters, and when creating the pod you can determine those parameters based on the tp.Config.Type?

There are subtle differences:
one waits to be ready, the other does not...,
one has REMOTE_PORT, the other LISTEN_PORT).
the sh lines are different.

I personally prefer separate functions to lots of "ifs" in the middle of the code.

Why bunch of IFs? You can have parameters you know..

yes but those parameters would need to acted on, wouldn't they?...

I probably not getting your idea. But also, I think there's no need to dive so much into detail, code is not written in stone, we can change it later specially since it is a private method now.

mkolesnik · 2019-10-29T12:56:06Z

test/e2e/framework/pods.go

+const (
+	InvalidPodType TestPodType = iota
+	ListenerPod
+	ConnectorPod
+)
+
+type TestPodScheduling int
+
+const (
+	InvalidScheduling TestPodScheduling = iota
+	GatewayNode
+	NonGatewayNode
+)


I feel that by moving this here you're mixing "pods" and "network_pods" which is a distinction that existed earlier, but now every pod is a "network pod" so I'm not sure if it makes sense to even have this code in separate files.

Yeah, I agree on that you're right, may be it's better to consolidate stuff. Non-network pods make no sense to our testing.

May be this file should be network_pods.go and the other one tcp_check_pods.go? or

pods.go + tcp_check_pods.go ?

How about we call it NetworkPod instead of TestPod and also the related types NetworkPod*? And put all the code in network_pods.go.

sounds good, I like your patch.

mkolesnik · 2019-10-29T13:02:07Z

test/e2e/framework/pods.go

 	Expect(err).NotTo(HaveOccurred())
-	return terminationCode, terminationMessage
+
+	return tp.Pod.Status.Phase == v1.PodSucceeded || tp.Pod.Status.Phase == v1.PodFailed


Wouldn't it have a different status only when it had an error (based on the switch on line 128)? In that case it would fail on line 138 anyway, making this check moot

That one (L128) returns to the inner loop of wait.PollInmediate

You could have still existed while the pod is pending, due to a timeout. In that case I want it to return "false",
because the Pod didn't really "Finish" and we are waiting for finish status.

I reckon that when timeout is reached it will return an error, in fact "ErrWaitTimeout is returned when the condition exited without success." from https://godoc.org/k8s.io/apimachinery/pkg/util/wait#pkg-variables

ha!, then the method wont work when waiting for timeouts :-/.

In the PR tom sent on top of this one he has modified that logic, and will only Await for successful... we can add a AwaitForFailure or AwaitForTimeout at a later time refactoring that one a bit.

- renamed TestPod to NetworkPod - renamed createListenerConnectorPair t0 runAndVerifyNetworkPod2ServicePair - renamed other function names where appropriate Signed-off-by: Tom Pantelis <tompantelis@gmail.com>

tpantelis · 2019-10-30T01:26:55Z

I pushed #197 to address my comments.

Jaanki

LGTM

mkolesnik · 2019-10-30T09:51:41Z

test/e2e/framework/network_pods.go

 // create a test pod inside the current test namespace on the specified cluster.
 // The pod will listen on TestPort over TCP, send sendString over the connection,
 // and write the network response in the pod  termination log, then exit with 0 status
-func (f *Framework) CreateTCPCheckListenerPod(cluster ClusterIndex, scheduling TestPodScheduling, sendString string) *v1.Pod {
+func (tp *TestPod) buildTCPCheckListenerPod() {


Why bunch of IFs? You can have parameters you know..

mkolesnik · 2019-10-30T11:44:52Z

test/e2e/framework/pods.go

 	Expect(err).NotTo(HaveOccurred())
-	return terminationCode, terminationMessage
+
+	return tp.Pod.Status.Phase == v1.PodSucceeded || tp.Pod.Status.Phase == v1.PodFailed


I reckon that when timeout is reached it will return an error, in fact "ErrWaitTimeout is returned when the condition exited without success." from https://godoc.org/k8s.io/apimachinery/pkg/util/wait#pkg-variables

* E2E: Refactor handling of pods The concept of TestPodConfig and TestPod is defined, letting us create testing pods from a single interface, and automating the creation of UUID data to exchange. As late modifications has shown that the number of parameters keeps growing having a single fixture to create test pods will make easier to write and read E2E tests. Depends-On: #148 * Address comments inn #149 - renamed TestPod to NetworkPod - renamed createListenerConnectorPair t0 runAndVerifyNetworkPod2ServicePair - renamed other function names where appropriate Signed-off-by: Tom Pantelis <tompantelis@gmail.com>

tpantelis · 2019-10-30T19:25:00Z

oops I mistakenly auto-rebased and merged #197 (I didn't pay attention to the browser window I had open :)). I meant to merge this one first - I guess it doesn't really matter since #197 has all the commits.

mangelajo · 2019-10-31T06:46:54Z

oops I mistakenly auto-rebased and merged #197 (I didn't pay attention to the browser window I had open :)). I meant to merge this one first - I guess it doesn't really matter since #197 has all the commits.

np :) we can close this one then.

mangelajo changed the title ~~E2e refactor podcreation~~ E2E refactor podcreation Sep 12, 2019

mangelajo requested review from skitt, tpantelis, Jaanki and sridhargaddam and removed request for skitt September 12, 2019 08:51

sridhargaddam requested changes Sep 15, 2019

View reviewed changes

mangelajo force-pushed the e2e-refactor-podcreation branch from 6168d2b to 5f3bdc3 Compare September 18, 2019 12:59

Merge branch 'master' into e2e-refactor-podcreation

d3d6d76

mpeterson approved these changes Sep 22, 2019

View reviewed changes

test/e2e/framework/network_pods.go Show resolved Hide resolved

mpeterson suggested changes Sep 22, 2019

View reviewed changes

Merge branch 'master' into e2e-refactor-podcreation

f638653

Merge branch 'master' into e2e-refactor-podcreation

8a858ba

mkolesnik requested changes Oct 29, 2019

View reviewed changes

tpantelis mentioned this pull request Oct 30, 2019

Address comments in #149 #197

Merged

Jaanki approved these changes Oct 30, 2019

View reviewed changes

mkolesnik requested changes Oct 30, 2019

View reviewed changes

tpantelis approved these changes Oct 30, 2019

View reviewed changes

mangelajo closed this Oct 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E refactor podcreation #149

E2E refactor podcreation #149

mangelajo commented Sep 12, 2019 •

edited

Loading

sridhargaddam Sep 15, 2019

mpeterson left a comment

mpeterson left a comment

mangelajo commented Sep 23, 2019

mangelajo commented Oct 27, 2019

mkolesnik Oct 29, 2019

tpantelis Oct 29, 2019

mkolesnik Oct 29, 2019

mangelajo Oct 29, 2019

mkolesnik Oct 30, 2019

mangelajo Oct 30, 2019

mkolesnik Oct 29, 2019

mangelajo Oct 29, 2019

tpantelis Oct 29, 2019

mangelajo Oct 30, 2019

mkolesnik Oct 29, 2019

mangelajo Oct 29, 2019

mkolesnik Oct 30, 2019

mangelajo Oct 30, 2019

tpantelis commented Oct 30, 2019

Jaanki left a comment

mkolesnik Oct 30, 2019

mkolesnik Oct 30, 2019

tpantelis commented Oct 30, 2019

mangelajo commented Oct 31, 2019


		func testPod2ServiceTCP(f *framework.Framework) {
		It("Should be able to perform a Pod to Service TCP connection and exchange data between different clusters NonGW to GW", func() {

E2E refactor podcreation #149

E2E refactor podcreation #149

Conversation

mangelajo commented Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

mpeterson left a comment

Choose a reason for hiding this comment

mpeterson left a comment

Choose a reason for hiding this comment

mangelajo commented Sep 23, 2019

mangelajo commented Oct 27, 2019

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

tpantelis commented Oct 30, 2019

Jaanki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpantelis commented Oct 30, 2019

mangelajo commented Oct 31, 2019

mangelajo commented Sep 12, 2019 •

edited

Loading