Fix flaky CI with a complex test case #114

nebojsa-prodana · 2021-07-02T11:18:10Z

As discussed in Infinite loop when multiple stage targeting the same clusters #103 , the https://github.com/Skyscanner/applicationset-progressive-sync/blob/main/controllers/progressivesync_controller_test.go#L268 test has a very naive ProgressiveSync object: two stages with a selector that returns only one cluster, and targeting a single cluster at the time.

We should change this test to better reflect a real case scenario. We've internally used as default the ProgressiveSync reported in #103 so this PR change the test to use that ProgressiveSync.

In addition, I changed a couple of helpers function to be more generic for when we want to add more tests with different combination of clusters.

TODO

~~Fix MaxTargets to return a percentange of the remaining Out Of Sync apps instead of all the apps~~ we will address this in Support maxTargets and maxParallel with % values #112 because it's a quite big change
Decomment scheduler tests
Decomment removeAnnotation

This is a fork of #104 on commit cab8fff

… stage

…d stage

…nctions

… want to sync

maruina · 2021-07-05T11:11:42Z

We found multiple business logic issues:

271996a when we now re-schedule for sync the progressing application as well so we can annotate them. This protect us against a case when someone is manually syncing from the ArgoCD UI and we were not annotating the app
40824ef we were missing a corner case in the scheduling logic
fab6f11 where we were not setting Requeue: true
if the labels selector returns an empty selection, meaning apps == nil, we decided to make the stage as completed

maruina · 2021-07-05T12:53:54Z

Makefile

@@ -23,7 +23,7 @@ lint: fmt vet

 # Run tests
 test: tools generate fmt vet manifests
-	ginkgo -r --randomizeAllSpecs --randomizeSuites --failOnPending --cover -coverprofile=../coverage.out --trace --race --progress
+	ginkgo -reportPassed -r --failOnPending --cover -coverprofile=../coverage.out --trace --race --progress


We need to remove `-reportPassed before merging

internal/scheduler/scheduler.go

DimitarHristov111

We reviewed it together yesterday and the logic seems fine. We still need to verify that it works as expected using the snapshot image. There are a few comments that came up during the review and I left them below.

controllers/progressivesync_controller_test.go

maruina · 2021-07-07T07:39:52Z

.github/workflows/test-build.yaml

+        with:
+          # list of Docker images to use as base name for tags
+          images: |
+            ghcr.io/skyscanner/applicationset-progressive-sync


We're now pushing to Github packages when opening a PR. This allow us to pull a specific docker image for e2e testing before merging it into main.

maruina · 2021-07-07T07:41:09Z

Makefile

@@ -23,7 +23,7 @@ lint: fmt vet

 # Run tests
 test: tools generate fmt vet manifests
-	ginkgo -r --randomizeAllSpecs --randomizeSuites --failOnPending --cover -coverprofile=../coverage.out --trace --race --progress
+	ginkgo -r --failOnPending --cover -coverprofile=../coverage.out --trace --race --progress


Remove the randomize options at least until we address #111
Until the controller isn't scoped to a single namespace, the ginkgo random options might cause different objects to pollute the test namespace.

maruina · 2021-07-07T07:42:02Z

controllers/progressivesync_controller.go


 	// Get the Applications to update
-	scheduledApps := scheduler.Scheduler(apps, stage)
+	scheduledApps := scheduler.Scheduler(log, apps, stage)


The scheduler logging as proved to be quite valuable. I propose we keep it in this PR and we fix-forward in #115

maruina · 2021-07-07T07:44:29Z

controllers/progressivesync_controller_test.go

@@ -745,10 +1104,10 @@ func TestSync(t *testing.T) {

 	testAppName := "foo-bar"

-	application, error := r.syncApp(ctx, testAppName)
+	application, err := r.syncApp(ctx, testAppName)


The IDE was complaining that error is a built-in package

controllers/progressivesync_controller.go

sledigabel

Some small comments, overall LGTM

controllers/progressivesync_controller.go

internal/utils/utils.go

sledigabel

LGTM!

maruina and others added 30 commits June 17, 2021 15:44

Fix linting errors because of missing variables

7e2b08f

Change the progressive sync to be a multi-region object

8c78895

Sync first stage

d3ea89c

Set second stage apps to progressing

f9edc07

Check the stages status while the second stage apps are progressing

7053531

Add a test for adding the annotation

c800475

Add a check to make sure the PS status is in progress after the first…

2c9caeb

… stage

Add a check to make sure the PS status is in progress after the secon…

4d1aec5

…d stage

Fix second stage cluster order

5e729e1

Add setAppHealthStatusProgressing and setAppStatusCompleted helper fu…

e882198

…nctions

Refactor second stage with helper functions

2c1e0cd

Add third stage

5c8c34b

Merge branch 'main' into new-test

939535e

Fix failed stage test

ab7e10f

WIP: update multi-stage test with syncedAt annotation

2da4f43

Eventual consistency for setting app status in tests

e250db4

Update IsStageComplete to take into account MaxTargets

e682b97

Add method to identify annotated apps

10488d8

WIP: Progress on resolving reconciliation loop bugs

9c0138d

Use single namespace and different application names

47b120c

Better variable names

c8ff093

Remove duplicate code

a16ada8

Remove ginkgo random stuff

a57231e

Add more informative log messages

08a6a51

Fix GetAppsName comment

8a15998

Fix IsStageInProgress by adding a condition on how many apps we still…

8ed805c

… want to sync

Try increasing the timeout

91ca302

Merge branch 'main' into new-test

3f7f213

Try increasing the timeout

df3afd7

Always log the progressive sync object

90a2bf3

maruina mentioned this pull request Jul 5, 2021

Add a more real case scenario reconcile test #104

Closed

3 tasks

maruina changed the title ~~Flaky ci~~ Fix flaky CI with a complex test case Jul 5, 2021

maruina reviewed Jul 5, 2021

View reviewed changes

Add Requeue: true along when setting RequeueAfter

4642e93

maruina mentioned this pull request Jul 5, 2021

Improve logging #115

Closed

3 tasks

Build a docker image on every PR

d7c84bd

maruina reviewed Jul 5, 2021

View reviewed changes

internal/scheduler/scheduler.go Show resolved Hide resolved

[skip ci] Minor tweaks in comments and remove the word rollout

047a4f9

DimitarHristov111 reviewed Jul 6, 2021

View reviewed changes

controllers/progressivesync_controller_test.go Outdated Show resolved Hide resolved

controllers/progressivesync_controller_test.go Show resolved Hide resolved

controllers/progressivesync_controller_test.go Outdated Show resolved Hide resolved

DimitarHristov111 mentioned this pull request Jul 6, 2021

Store the annotated apps in memory #116

Merged

maruina added 4 commits July 7, 2021 09:27

Remove PS deletion at the end of test

4ae4393

Make sure the previous stages stay completed when starting the last one

b7b24c4

Consistenly use Parse to define an intstr as it supports %

fa99700

Remove reportPassed option from ginkgo

e58bbb3

maruina reviewed Jul 7, 2021

View reviewed changes

maruina added 2 commits July 7, 2021 09:45

[skip ci] Add Target struct description

d237afb

Merge branch 'main' into flaky-ci

2e7df69

sledigabel reviewed Jul 7, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

sledigabel reviewed Jul 7, 2021

View reviewed changes

controllers/progressivesync_controller.go Outdated Show resolved Hide resolved

Use correct application in logs

7f911fb

sledigabel reviewed Jul 7, 2021

View reviewed changes

controllers/progressivesync_controller.go Outdated Show resolved Hide resolved

controllers/progressivesync_controller.go Show resolved Hide resolved

controllers/progressivesync_controller.go Show resolved Hide resolved

internal/utils/utils.go Show resolved Hide resolved

maruina added 2 commits July 8, 2021 09:18

Declare requeueDelayOnError as constant

700200c

Restore walrus operator

aa40f44

sledigabel approved these changes Jul 8, 2021

View reviewed changes

maruina merged commit 7c57da0 into main Jul 8, 2021

maruina deleted the flaky-ci branch July 8, 2021 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky CI with a complex test case #114

Fix flaky CI with a complex test case #114

nebojsa-prodana commented Jul 2, 2021 •

edited by maruina

maruina commented Jul 5, 2021 •

edited

maruina Jul 5, 2021

DimitarHristov111 left a comment

maruina Jul 7, 2021

maruina Jul 7, 2021

maruina Jul 7, 2021

maruina Jul 7, 2021

sledigabel left a comment

sledigabel left a comment

Fix flaky CI with a complex test case #114

Fix flaky CI with a complex test case #114

Conversation

nebojsa-prodana commented Jul 2, 2021 • edited by maruina

maruina commented Jul 5, 2021 • edited

maruina Jul 5, 2021

Choose a reason for hiding this comment

DimitarHristov111 left a comment

Choose a reason for hiding this comment

maruina Jul 7, 2021

Choose a reason for hiding this comment

maruina Jul 7, 2021

Choose a reason for hiding this comment

maruina Jul 7, 2021

Choose a reason for hiding this comment

maruina Jul 7, 2021

Choose a reason for hiding this comment

sledigabel left a comment

Choose a reason for hiding this comment

sledigabel left a comment

Choose a reason for hiding this comment

nebojsa-prodana commented Jul 2, 2021 •

edited by maruina

maruina commented Jul 5, 2021 •

edited