[STRMCMP-1639] Integration Test Fix #277

sethsaperstein-lyft · 2023-03-21T23:38:22Z

overview

Integrations tests working in CI again

Additional Info

Change to minikube from microk8s. Largely due to being able to reproduce the integration env locally. Iterating bugs that occurred only in CI on microk8s due to memory was inefficient.
Upgrade k8s to 1.20 from 1.13. Changes for v1 CRD's were introduced to support 1.22 k8s however k8s client, on 1.14, for integration tests only support v1beta1 which was deprecated in 1.22. Upgrading the client is non-trivial and will be done separately. Using older k8s versions met issues with docker and local system cgroup. 1.20 became the decided upon version.
Create service account, role, and role binding in CI since 1.20 has RBAC by default. Disabling rbac is hacky and resulted in issues with the kube proxy
Remove base mount of tmp directory due to permission issues. Flink application writes 0755. Integration test downgrades 0777 to 0755. Using minikube ssh directly to create and delete the necessary files. This creates a strong dependency on minikube which is acceptable for now.
Comment out test for rescale given that default github runner only has 8gb memory and 2 cpu. Rescale needs 2 flink clusters up with above average size which busts limits. Separate ticket created to address using larger runners (in beta). Flink test apps were also scaled down to parallelism 2 using 1 TM to fit on the github runner
Changes made to build the test app image locally as opposed to pull from dockerhub. Not in use currently as the tests are remaining on 1.8 due to simpler memory configuration until larger github runners are used. Additional changes made to upgrade flink app from 1.8 to 1.11.
Upgrade ubuntu for CI as ubuntu version is deprecated on april 1st 2023 by github

integ/README.md

leoluoInSea · 2023-03-29T23:27:56Z

integ/README.md

+8. Set the following for the Go test:
+   Package path: github.com/lyft/flinkk8soperator/integ
+   Env: INTEGRATION=true;OPERATOR_IMAGE=flinkk8soperator:local;RUN_DIRECT=true
+   Program Args: -timeout 40m -check.vv IntegTest


I thought IntegTest is not needed.

I removed this when attempting to target a single test. No luck there for now so leaving as is since this is what was there for the integration test prior. In the future we'll figure out how to target a single test as I'll likely write an integration test for fallback without savepoint

premsantosh · 2023-03-30T15:27:10Z

integ/README.md

+is non-trivial due to cgroup configurations. Instead, we will use a version
+that is compatible with v1beta1 CRD's which corresponds to <1.22. CRD's v1
+is only available with client >=1.16, however, the client used here is 1.14
+and the upgrade is non-trivial.


Maybe just add a TODO here with the jira to upgrade client?

Created a ticket for the 1.22 upgrade and added

premsantosh · 2023-03-30T15:32:36Z

integ/checkpoint_failure_test.go

 	failingJobTest(s, c, "taskfailure", func() {
-		f, err := os.OpenFile(s.Util.CheckpointDir+"/fail", os.O_RDONLY|os.O_CREATE, 0666)
+		err := s.Util.ExecuteCommand("minikube", "ssh", "touch /tmp/checkpoints/fail && chmod 0644 /tmp/checkpoints/fail")


I don't get this test, you're creating and modifying privs on a file and then just asserting the commands ran without errors?

Changes the behavior of the flink app: https://github.com/lyft/flinkk8soperator/blob/master/integ/operator-test-app/src/main/java/com/lyft/OperatorTestApp.java#L92

premsantosh · 2023-03-30T15:36:09Z

integ/job_cancellation_test.go

@@ -163,7 +166,7 @@ func (s *IntegSuite) TestCancelledJobWithoutSavepoint(c *C) {
 	c.Assert(err, IsNil)

 	// wait a bit
-	time.Sleep(1 * time.Second)
+	time.Sleep(5 * time.Second)


Don't have to change anything but generally speaking I am never a fan of such sleep timers in tests (just makes it flakey).
I would always promote to change this to an polling method with exponential delay (and an exit criteria). Again, don't need to do that now but just wanted to point it out.

Agreed. Will leave as a future update or at least keep in mind while writing future integration tests for this repo

premsantosh · 2023-03-30T16:01:16Z

integ/scaleup_test.go

-	log.Info("All pods torn down")
-}
+// TODO: https://github.com/lyft/flinkk8soperator/issues/278
+//func (s *IntegSuite) TestInPlaceScaleUp(c *C) {


Instead of commenting all this out I believe you can just do
c.Skip("Skipping because of blah")

added! Didn't know this existed! Thx!

leoluoInSea · 2023-03-30T16:09:52Z

Tons of thanks for tackling down complicated problem!

sethsaperstein-lyft · 2023-03-31T18:11:56Z

/PTAL @leoluoInSea

sethsaperstein-lyft added 30 commits March 21, 2023 16:36

local test working for TestJobCancellationWithoutSavepoint

a0a90a0

comment back in tests

0cc931c

waits between cancel

3859dc3

5 min timeout

33234ca

additional logging

a43addf

remove local debug changes

698bc34

test app on flink 1.11

4e81a8d

remove local changes

cb21077

fix docker location

8a7811b

move to proper dir

68b2330

break waiting after 30s so errors show

b0eede8

log all events at failure

490f385

forgot to add microk8s docker build and push

f821e91

wait longer for running app

3e6dbfe

tm logs first

5101df2

see if microk8s can take the memory

e904616

bump memory for tm due to 20mb allocated to task heap

0196be5

increase flink tm heap size

abc7186

set memory param. likely oom on second cluster

3965f45

fix typo

74bebca

change memory configs

bca42b4

add limits to containers. add concurrency limit to go test

02ebaef

limit cpu

c469f10

describe k8s resources on failure

bd5a7b7

add get pods to view restarts

b3fabc6

try with minikube

f425da7

flink 1.8 not 1.11

5350c37

make executable

3110b64

remove microk8s from proxy kube

f6cc040

fix minikube command

15787c6

sethsaperstein-lyft added 5 commits March 29, 2023 16:26

after cluster start

3db9f0c

check if kube dir exists

6e86f70

set kube config location

7fab7cc

check kube config env var

11fbbe8

move env var to test.sh since sudo

69ccd27

leoluoInSea reviewed Mar 30, 2023

View reviewed changes

sethsaperstein-lyft added 5 commits March 29, 2023 17:17

update docs

21b7f7b

upgrade newer ubuntu. all setup sudo so home is same

2579200

run tests without sudo

48b2ecf

fix format

aba457f

remove commented env var

db88cf9

sethsaperstein-lyft marked this pull request as ready for review March 30, 2023 13:05

sethsaperstein-lyft requested review from anandswaminathan, premsantosh and maghamravi as code owners March 30, 2023 13:06

sethsaperstein-lyft changed the title ~~[TEST] Integration Test Fix~~ Integration Test Fix Mar 30, 2023

sethsaperstein-lyft changed the title ~~Integration Test Fix~~ [STRMCMP-1639] Integration Test Fix Mar 30, 2023

premsantosh previously approved these changes Mar 30, 2023

View reviewed changes

leoluoInSea previously approved these changes Mar 30, 2023

View reviewed changes

anandswaminathan previously approved these changes Mar 31, 2023

View reviewed changes

jira and test skip fix

1db3074

sethsaperstein-lyft dismissed stale reviews from anandswaminathan, leoluoInSea, and premsantosh via 1db3074 March 31, 2023 17:59

sethsaperstein-lyft requested a review from leoluoInSea March 31, 2023 18:08

leoluoInSea approved these changes Mar 31, 2023

View reviewed changes

sethsaperstein-lyft merged commit d3a6fe4 into master Mar 31, 2023

sethsaperstein-lyft deleted the integration_test_fix branch March 31, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STRMCMP-1639] Integration Test Fix #277

[STRMCMP-1639] Integration Test Fix #277

sethsaperstein-lyft commented Mar 21, 2023 •

edited

Loading

leoluoInSea Mar 29, 2023

sethsaperstein-lyft Mar 30, 2023

premsantosh Mar 30, 2023

sethsaperstein-lyft Mar 31, 2023

premsantosh Mar 30, 2023

sethsaperstein-lyft Mar 31, 2023

premsantosh Mar 30, 2023

sethsaperstein-lyft Mar 31, 2023

premsantosh Mar 30, 2023

sethsaperstein-lyft Mar 31, 2023

leoluoInSea commented Mar 30, 2023

sethsaperstein-lyft commented Mar 31, 2023

[STRMCMP-1639] Integration Test Fix #277

[STRMCMP-1639] Integration Test Fix #277

Conversation

sethsaperstein-lyft commented Mar 21, 2023 • edited Loading

overview

Additional Info

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

leoluoInSea commented Mar 30, 2023

sethsaperstein-lyft commented Mar 31, 2023

sethsaperstein-lyft commented Mar 21, 2023 •

edited

Loading